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 _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev