Re: [lmi] Request for review

[Vadim Zeitlin]

On Wed, 11 Jan 2017 23:23:56 +0000 Greg Chicares <address@hidden> wrote:

GC> I'm trying to figure out a solid universal rule for this modernization.
GC> What do you think about transforming:
GC> 
GC>   for(const_iterator i...)   -->   for(auto const& k: ...)
GC>   for(      iterator i...)   -->   for(auto      & k: ...)
GC> 
GC> That seems simple and logical, to me at least.

 Yes, it does to me too, and this is what I basically used for the changes of
https://github.com/vadz/lmi/pull/52

GC> But it doesn't quite fit our practice so far: we have ranged for-loops
GC> only in 'rate_table*', and...
GC> 
GC> $grep --no-filename 'for(.*: ' *.?pp |sed -e's/^ *//' |sort |less -S
GC> for(auto const& i: index_)
GC> for(auto const& j: table_names)
GC> for(auto const& not_field: known_not_fields)
GC> for(auto num: numbers)
GC> for(auto num: numbers)
GC> for(auto num: numbers)
GC> for(auto v: values)
GC> for(auto v: values)
GC> for(auto v: values)
GC> for(auto v: values_)
GC> for(auto& e: index_by_number_)
GC> for(auto& v: values_)
GC> for(soa_field const& f: soa_fields)
GC> 
GC> Skipping the last line because it lacks 'auto'

 No idea why, to be honest. I think it should use "auto" too...

GC> I count:
GC>  - 3  auto const&
GC>  - 2  auto&
GC>  - 7  auto [neither const nor &]
GC> What was your rule for choosing plain 'auto'? I'm guessing you used
GC> it in cases where 'auto const&' would have been just as good, but
GC> where you knew that using a copy would involve no more overhead than
GC> using a const reference?

 Exactly, I used plain "auto" for "int" or "double" as it seemed weird to
work with them by (const) reference. For the changes of the PR 52 I chose a
slightly different tack and used "auto&" or "auto const&" in general and
the primitive type (and not "auto") for this case.

 Reviewing the changes now I see that I wasn't 100% consistent,
unfortunately, e.g. I used "auto" instead of "wxWindowID" (which is just an
int) in one place and also instead of "unsigned char" in another one and
instead of "wxWindow*" a few more times. I probably should fix this...

GC> Let me ask that another way: if I were hypothetically to propose
GC> changing all those naked auto's to 'auto const&', what objection
GC> would you have, if any?

 No real objections, but it looks like a premature pessimization to me.
It's quite possible that modern compilers will manage to optimize it and
produce the same code even so (I could test this...), but just not using
"const int&" when a plain "int" would do seems more natural to me.

 Regards,
VZ