[squid-dev] Fwd: [PATCH] for loops modernization

[Amos Jeffries]

On 15/03/2017 11:43 p.m., khaled belhout wrote:
> hello Alex
> I fixed the naming of the current iteration object, limited the
> changes to "src/adaptation" sub tree and replaced NULL macro by
> nullptr keyword.
> 

Hi Khaled,

Thanks for the proposals and especially for identifying a tool to detect
the changes automatically.

I would *love* to have all this change away in the past, but I am
(reluctantly) in agreement with Alex regarding these patches.


For several reasons (in no particular order):

2) We have had a lot of discussions about large cleanup like this. While
I am personally in favour of them, there are some very good reasons -
specifically around backporting as Alex metioned.

The current compromise we have in the team is to include this type of
cleanup in all patches - code which gets changed for other reasons gets
polished as well. And in a somewhat slow drip of maintenance changes
done for the C++ conversion which is ongoing since 3.0.

That said, there are points in the lifecycle which are more friendly to
bg changes. The next one I expect these C++11 changes can even have a
chance is after 3.5 ceases to be a supported version. Until that
milestone is reached we have a lot of people relying on 3.5 backports
being reliable and older compilers building the code. Yes that does
cramp the C++11 conversion a bit.


1) Size versus content of the patches.

In a large codebase like Squid there are a lot of hidden dependencies
and logic nobody has read for decades.

In these conditions it is better to isolate each specific polishing task
in its own patch. That way the big changes have a singular, explicit and
clear effect on the code behaviour which can be described in the merge
comment. The audit for large patches is already mind-numbing and not
having to check multiple effects at a time helps a LOT.


3) Not matching our actual needs.

We already have a pretty good idea of what we want the cleanup up code
to look like. It is just the above situations get in the way. The
changes this tool is doing do not go far enough to be what we consider
clean code. There are extra actions which appear to still be manual and
necessary to get that end product.

For example;  running sed -e 's/NULL/nullptr/g' is easy, and the reverse
can be done on 3.5 patches. If that were what we wanted the v4+ code to
look like it would have happened already. We want to do better.

For code to be accepted into Squid nowdays NULL is sometimes replaced
with nullptr, but usually in boolean conditions involving a pointer (or
Pointer object) the equivalence should be dropped entirely.
 if (x == NULL) becomes if (!x), etc.

We have a history of getting constructor initialization lists overly
verbose or incomplete, sometimes both at once. So with C++11 I am also
working towards default initialization for classes which adds another
complication for NULL removal.  That cleanup change means moving the
location of the nullptr (and other trivial types) to the .h definition
and then deciding whether the constructor can be dropped entirey, or
remains as inline (or not) and/or with much shortened initializer list.


* There are some things like HERE macro which are obsolete and should be
simply erased.

Although again with HERE a manual analysis of each code block is needed
to make sure they still make sense. Most can be dropped but certain
messages need changing to use MYNAME instead. And many debugs() texts
predate even the HERE macro itself and need string edits to remove
explicit nameing of a function (which might not be the function/method's
current name! yuck).

... and some other projects cleaning up the code style as much as we can
and removing outdated C object types.


So we have the current policy of when code is touched it gets polished
up according to all the waiting 'cleanup' requirements.

Plus an additional stream of small targeted cleanup patches is going in
all the time in a way that can be audited relatively quickly (if
necessary) and issues identified quickly if there are any.
 You will mostly see these by me at present and mentioning "Cleanup:" or
C++11 in the patch descriptions.



Getting back to your patch submission specifically:

Theoretically range-for loops should allow multi-threaded CPU to run
those loops a bit faster. If that can be demonstrated using a tool like
polygraph you have a good argument for a patch containing that change to
go in as a pure performance change.

Without that evidence or if testing shows no speed gain because compiler
optimization is good - then it is just a very large polish patch and we
have to treat it as such with a "sorry, not now".


Amos