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

[Alex Rousskov]

On 03/15/2017 04:43 AM, khaled belhout wrote:

> I fixed the naming of the current iteration object, 

Thank you. FWIW, it is not necessary to add _ptr and similar suffixes to
object names when the fact that it is a pointer (or a reference, or a
copy) is unimportant.


> limited the changes to "src/adaptation" sub tree

Why?!


> and replaced NULL macro by nullptr keyword.

You should not have:

* Due to the same porting overhead concerns that I was trying to explain
earlier, we only replace NULLs when the code needs to be changed for
other reasons.

* Mixing benign NULL and potentially deadly (for performance or
const-correctness) auto changes complicates review.


BTW, I am not sure, but I believe the updated patch still seems to be
missing "const" in at least one "auto" conversion case. I did not check
all of them because of the NULL replacement noise.


In summary: I still do not think these changes are desirable (even in
their polished variant), but if others overrule me on that, my comments
about the quality of the current changes still apply.


Thank you,

Alex.


> 2017-03-13 14:45 GMT+01:00 Alex Rousskov <rousskov at measurement-factory.com>:
>> On 03/12/2017 08:42 PM, khaled belhout wrote:
>>> I used clang-tidy tool to modernize all these loops.
>>> http://clang.llvm.org/extra/clang-tidy/
>>
>> It looks like that tool is not ready for fully automated use. If you
>> want to fix its results, please use "const auto" where possible and
>> avoid using "i" for naming the current iteration object.
>>
>>
>>> we can take the advantage of the tool by selecting the changes that
>>> make code more readable understandable and maintainable.
>>
>> Please do not misinterpret my earlier comments as an argument against
>> range loops. Range loops are good and new code should use them!
>>
>> However, I doubt the advantages of changing those old loops outweigh
>> cross-branch development costs right now. Others may disagree, and, if
>> they do, I would not object to a polished patch being committed.
>>
>>
>> Thank you,
>>
>> Alex.
>>
>>
>>> 2017-03-12 16:31 GMT+01:00 Alex Rousskov:
>>>> On 03/12/2017 07:45 AM, khaled belhout wrote:
>>>>
>>>>> this patch modernize for loops using c++11 Range-based for loop
>>>>
>>>> Please use "const auto" where possible and avoid using "i" for naming
>>>> the current iteration object.
>>>>
>>>> I am curious why did you decide to change all these loops? How did you
>>>> select the loops to change? Normally, we avoid wide-spread polishing
>>>> touches to minimize the price developers working with older code have to
>>>> pay when porting back various bug fixes and features. I am trying to
>>>> decide whether the advantages of changing these loops outweigh those
>>>> costs in this case.
>>