Re: [lmi] Request for review

[Vadim Zeitlin]

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.

GC> But I'm still not comfortable using the...well, whatever 'i' is,
GC> because I guess it's not an iterator,

 Personally I find it very helpful to get back to "range-based for"
definition from http://en.cppreference.com/w/cpp/language/range-for (which
closely copies that from the standard) using which we can see that

        for(auto i: ScalableVectors)
            {
            ...
            }

is exactly the same as

        auto && __range = range_expression ;
        for(auto __begin = ScalableVectors.begin(), __end = 
ScalableVectors.end();
            __begin != __end; ++__begin)
            {
            auto i = *__begin;
            ...
            }

GC> as it has already been dereferenced. At least I'm not yet
GC> comfortable enough to do that in my sleep and feel really confident
GC> that I didn't get it wrong, so 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.

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, e.g. here is
a diff fragment from the same file:

---------------------------------- >8 --------------------------------------
-    for
-        (scalar_map::iterator i = AllScalars.begin()
-        ;i != AllScalars.end()
-        ;i++
-        )
+    for(auto& i: AllScalars)
         {
-        *(*i).second = 0.0;
+        *i.second = 0.0;
         }
---------------------------------- >8 --------------------------------------

 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. I did promise
to stop working on this, but this is something that I had mostly already
done a long time ago...

GC> In order to gain the confidence I still lack, I searched online for
GC> a guide that someone might have written about migration to C++11.
GC> While I didn't find what I sought, I did find something that might
GC> be even better--a tool that does the work, and not one provided by
GC> some mere dilettante either:
GC> 
GC> http://blog.llvm.org/2013/04/status-of-c11-migrator.html
GC> 
GC> Have you used it?

 No, but this looks very similar to what clang-tidy does and this was, of
course, what I used to do most of the heavy lifting so far. I think this
tool might have been superseded by clang-tidy in the meanwhile.

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.


 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.

 Thanks in advance,
VZ