Re: [lmi] [PATCH] Add a cast to fix compilation in 64 bits

[Vadim Zeitlin]

On Mon, 13 Mar 2017 04:36:10 +0000 Greg Chicares <address@hidden> wrote:

GC> First, however:
GC> 
GC> > ---------------------------------- >8 
--------------------------------------
GC> >>From 3526d20b7f4c7ba15340981ff21b1e2a1384e2ce Mon Sep 17 00:00:00 2001
GC> > From: Vadim Zeitlin <address@hidden>
GC> 
GC> Two "From" lines, the first one quoted? I wondered how icedove could have
GC> done that, but it appears that way in the html archives:
GC> 
GC> http://lists.nongnu.org/archive/html/lmi/2017-03/msg00021.html

 Sorry, this one was indeed my fault, I shouldn't have included this patch
inline as "From " line does get quoted on sending end in this case. In the
future I'll send all the patches to be applied with git-am as attachments.

GC> Okay, now that it compiles, how should we rewrite it for clarity?
GC> 
GC> Even after all these years, I still think in APL, so my first idea is
GC>   data ← original_length ↑ shorter_length ↑ data
GC> thus:
GC> 
GC>     ledger_map_t const& l_map_rep = ledger_map_->held();

 This is minor, but I don't think we need this variable, we could just iterate
directly over the RHS.

GC>     using T = decltype(ledger_invariant_->InforceLives)::size_type;
GC>     T original_size = ledger_invariant_->InforceLives.size();
GC>     T lapse_year = T(0);
GC>     for(auto const& i : l_map_rep)
GC>         {
GC>         lapse_year = std::max(lapse_year, 
static_cast<T>(i.second.LapseYear));
GC>         }
GC>     if(lapse_year < original_size)
GC>         {
GC>         ledger_invariant_->InforceLives.resize(lapse_year);
GC>         ledger_invariant_->InforceLives.resize(original_size);
GC>         }
GC> 
GC> (I don't see a much less annoying way to get size_type; ptrdiff_t isn't
GC> guaranteed to work forever, and I don't ever want to have to correct
GC> this type again.)

 What about this:

        auto original_size = ledger_invariant_->InforceLives.size();
        decltype(original_size) lapse_year{0};

? This would require changing the assignment to lapse_year inside the loop
to either cast lapse_year to double or use "decltype(lapse_year)" instead
of "T" in the currently written cast, but I still prefer using "auto var =
..." to "using T = decltype(...); T var = ..." if only because it avoids
repeating "...".

 Or perhaps lapse_year should use the "correct" type in the first place,
i.e. the same as LedgerVariant::LapseYear, i.e. "double". Then the call to
std::max wouldn't require any casts and we'd just need to cast it once to
the decltype(original_size) at the end.


GC> Is that really likely to be far short of optimally efficient?

 Frankly, I don't think we care about efficiency at all here. AFAICS the
typical number of elements in this vector is just "a few" and maximal
number if probably not more than a hundred (but if it were a thousand, it
still wouldn't change anything). So for me what should count most here is
the clarity of the code.

GC> resize() doesn't reduce the vector's capacity, so there's no memory
GC> allocation. Sure, we're destroying the last (original_size-lapse_year)
GC> elements and default-inserting the same number, but notionally at least
GC> that's no more complicated than explicitly zeroing the elements.

 But is this really more clear than using std::fill()? I don't think so,
e.g. I'd definitely insert a comment saying that the 2 resize() calls above
are used in order to zero the existing elements and not to resize the
vector at all. So I'd simply write

        if(lapse_year < original_size)
            {
            std::fill(lives.begin() + lapse_year, lives.end(), 0.0);
            }

which is IMHO clear enough ("lives" is, of course, just a helper defined in
as "auto& lives = ledger_invariant_->InforceLives").

 Regards,
VZ