Re: [lmi] Request for review

[Greg Chicares]

On 2017-01-11 20:38, Vadim Zeitlin wrote:
> On Wed, 11 Jan 2017 17:59:50 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> Vadim--Ironically enough, here's a change that I've only tested, so I
> GC> know it actually DTRT; but I haven't proven that it's correct.
> GC> 
> GC> I'm pretty sure this part is correct:
> GC> -    for
> GC> -        (double_vector_map::iterator svmi = ScalableVectors.begin()
> GC> -        ;svmi != ScalableVectors.end()
> GC> -        ;svmi++
> GC> -        )
> GC> +    for(auto i: ScalableVectors)
> GC>          {
> GC> because, well, how could it be wrong?
> 
>  It's indeed not wrong but it is slightly suboptimal as it does copy the
> map value_type objects, instead of just referencing them. It's also rather
> confusing to not use references when modifying the elements, it does work
> here because the modification happens via a pointer, but it's not very
> clear.

Thanks for the explanation. This has been bothering me. I have demonstrated
that it performs correctly, but I couldn't see the reason why. (Pointers.)

I'm trying to figure out a solid universal rule for this modernization.
What do you think about transforming:

  for(const_iterator i...)   -->   for(auto const& k: ...)
  for(      iterator i...)   -->   for(auto      & k: ...)

That seems simple and logical, to me at least. But it doesn't quite fit our
practice so far: we have ranged for-loops only in 'rate_table*', and...

$grep --no-filename 'for(.*: ' *.?pp |sed -e's/^ *//' |sort |less -S
for(auto const& i: index_)
for(auto const& j: table_names)
for(auto const& not_field: known_not_fields)
for(auto num: numbers)
for(auto num: numbers)
for(auto num: numbers)
for(auto v: values)
for(auto v: values)
for(auto v: values)
for(auto v: values_)
for(auto& e: index_by_number_)
for(auto& v: values_)
for(soa_field const& f: soa_fields)

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

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

> GC> [...] would you please review this line:
> GC> 
> GC> +        *(i).second *= m_scaling_factor;
> 
>  So "i" here is ScalableVectors::value_type and its .second is the
> std::vector<double>* stored in this map element, hence the line does seem
> to do what it means to.

A std::map of pointers to std::vectors. Well, the other day someone sent
me a copy of some BASIC code I wrote on 1988-09-12, and that was ugly too.

So in this case a naked 'auto' is not incorrect (because it makes a copy
of a pointer, which is not costlier than passing a pointer by reference),
but that doesn't make it righteous.

> GC> ? (On second thought, could "auto i" be wrong--is "auto& i" wanted
> GC> here instead?)
> 
>  Yes, this is what I used in my existing changes to this file
[...]
>  Which might be a good opportunity to mention that these changes, i.e.
> replacing of the loops over all container elements with range-based for
> statements, was my last outstanding patch that I wanted to submit. I didn't
> have time to test it fully yet, but if you can wait just a little bit, I
> could do it today/tomorrow and create a pull request for it.

Yes, please.

> GC> I wonder if I might also have your thoughts on the section commented
> GC>   // TODO ?? std::transform() might be cleaner.
> GC> in the original (which might even predate C++98). Taking a quick
> GC> glance at it, I think std::accumulate() might have been meant; but,
> GC> if we C++11-ify those loops, e.g. [untested]:
> GC> 
> GC>   for(auto i: AllVectors) {crc += i->second;}
> GC> 
> GC> does that really cry out to be rewritten with an STL algorithm?
> 
>  IMO, absolutely not, see my thoughts about std::for_each() in another
> thread.

That is very good to hear. For about two decades I've been thinking
that I should use for_each() a lot more (maybe only because that
algorithm's name makes it seem so compelling), but every time I try
to use it, it feels wrong.

>  Please let me know if I should try to submit my range-based for changes
> a.s.a.p. or whether it's already too late.

It is not too late, and that would be most welcome.