Re: [lmi] Request for review

[Vadim Zeitlin]

On Thu, 12 Jan 2017 13:03:10 +0000 Greg Chicares <address@hidden> wrote:

GC> Rule 5: Always write ranged-for loops as 'for(auto&& ...'.
GC> 
GC> ...which is my new favorite.

 Sorry, I have to strongly disagree with this one. I really like to have an
immediate visual indication that the container being iterated over is
changed from the loop and "auto&&" doesn't give it. On a tangential note,
as much as I appreciate STL's posts/ideas/cat utterances, I am glad his
N3853 proposal was rejected, notwithstanding the official reason (shadowing
of the variable with the same name outside the loop) IMO it was not worth
the resulting loss of readability neither.

 In addition to my main problem with "auto&&" above, I also think it's
significantly less clear what does it stand for. Both "auto&" and "auto
const&" are, I think, pretty clear but "auto&&" can be much more difficult
to resolve.

 Of course, I also don't like

GC> Rule 4: Always write ranged-for loops as 'for(auto& ...'.

for the same reason.

 So the only rule which looks reasonable to me is:

GC> Rule 2:
GC> - for(std::vector<T>::iterator ...
GC> + for(auto& ...
GC> - for(std::vector<T>::const_iterator ...
GC> + for(auto const& ...

 The main objection I had to doing this was that it could pessimize
generated code, but at least with the very simple loops I tried there is no
difference between using "double" or "auto const&" in the code iterating
over std::vector<double> or using "char" or "auto const&" in the code
iterating over std::string, the assembler output generated by g++ 4.9 is
exactly the same:

        % md5sum forvec*.s
        6f9f7f99d0fead8c578759a336faa67d  forvecdouble.s
        6f9f7f99d0fead8c578759a336faa67d  forvecauto.s

        % md5sum forstr*.s
        fcee3c67be7e166ca37fc1f2540894da  forstrauto.s
        fcee3c67be7e166ca37fc1f2540894da  forstrchar.s

 So it doesn't seem like we need to care about this and the rule 2 is
indeed the best choice.

 Thanks for making me check my assumptions about this,
VZ