lmi
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[lmi] Request for review


From: Greg Chicares
Subject: [lmi] Request for review
Date: Wed, 11 Jan 2017 17:59:50 +0000
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.5.1

Vadim--Ironically enough, here's a change that I've only tested, so I
know it actually DTRT; but I haven't proven that it's correct.

I'm pretty sure this part is correct:
-    for
-        (double_vector_map::iterator svmi = ScalableVectors.begin()
-        ;svmi != ScalableVectors.end()
-        ;svmi++
-        )
+    for(auto i: ScalableVectors)
         {
because, well, how could it be wrong? But I'm still not comfortable
*using* the...well, whatever 'i' is, because I guess it's not an
iterator, as it has already been dereferenced. At least I'm not yet
comfortable enough to do that in my sleep and feel really confident
that I didn't get it wrong, so would you please review this line:

+        *(i).second *= m_scaling_factor;

? (On second thought, could "auto i" be wrong--is "auto& i" wanted
here instead?)

In order to gain the confidence I still lack, I searched online for
a guide that someone might have written about migration to C++11.
While I didn't find what I sought, I did find something that might
be even better--a tool that does the work, and not one provided by
some mere dilettante either:

http://blog.llvm.org/2013/04/status-of-c11-migrator.html

Have you used it?

I wonder if I might also have your thoughts on the section commented
  // TODO ?? std::transform() might be cleaner.
in the original (which might even predate C++98). Taking a quick
glance at it, I think std::accumulate() might have been meant; but,
if we C++11-ify those loops, e.g. [untested]:

  for(auto i: AllVectors) {crc += i->second;}

does that really cry out to be rewritten with an STL algorithm?

--------8<--------8<--------8<--------8<--------8<--------8<--------8<--------

diff --git a/ledger_base.cpp b/ledger_base.cpp
index 273e646..682947a 100644
--- a/ledger_base.cpp
+++ b/ledger_base.cpp
@@ -26,13 +26,12 @@
 #include "alert.hpp"
 #include "assert_lmi.hpp"
 #include "crc32.hpp"
+#include "et_vector.hpp"
 #include "miscellany.hpp"               // minmax
 #include "value_cast.hpp"
 
 #include <algorithm>
 #include <cmath>                        // std::pow()
-#include <functional>
-#include <numeric>
 #include <string>
 
 //============================================================================
@@ -459,23 +458,9 @@ void LedgerBase::ApplyScaleFactor(double a_Mult)
         }
     m_scale_unit = look_up_scale_unit(m_scaling_factor);
 
-    // TODO ?? Would be clearer with bind1st.
-    std::vector<double>M(GetLength(), m_scaling_factor);
-    for
-        (double_vector_map::iterator svmi = ScalableVectors.begin()
-        ;svmi != ScalableVectors.end()
-        ;svmi++
-        )
+    for(auto i: ScalableVectors)
         {
-        // ET !! *(*svmi).second *= M;
-        std::vector<double>& v = *(*svmi).second;
-        std::transform
-            (v.begin()
-            ,v.end()
-            ,M.begin()
-            ,v.begin()
-            ,std::multiplies<double>()
-            );
+        *(i).second *= m_scaling_factor;
         }
 }
 
@@ -494,7 +479,7 @@ double LedgerBase::ScaleFactor() const
 //============================================================================
 void LedgerBase::UpdateCRC(CRC& crc) const
 {
-// TODO ?? std::transform() might be cleaner.
+// TODO ?? std::transform() might be cleaner. [Really?]
     for
         (double_vector_map::const_iterator vmi = AllVectors.begin()
         ;vmi != AllVectors.end()

-------->8-------->8-------->8-------->8-------->8-------->8-------->8--------



reply via email to

[Prev in Thread] Current Thread [Next in Thread]