[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--------
- [lmi] Request for review,
Greg Chicares <=
- Re: [lmi] Request for review, Vadim Zeitlin, 2017/01/11
- Re: [lmi] Request for review, Greg Chicares, 2017/01/11
- Re: [lmi] Request for review, Vadim Zeitlin, 2017/01/11
- Re: [lmi] Request for review, Greg Chicares, 2017/01/12
- Re: [lmi] Request for review, Greg Chicares, 2017/01/12
- Re: [lmi] Request for review, Greg Chicares, 2017/01/12
- Re: [lmi] Request for review, Vadim Zeitlin, 2017/01/12
- Re: [lmi] Request for review, Greg Chicares, 2017/01/12
- Re: [lmi] Request for review, Vadim Zeitlin, 2017/01/12
- Re: [lmi] Request for review, Greg Chicares, 2017/01/12