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

[Greg Chicares]

On 2017-03-05 20:05, Vadim Zeitlin wrote:
> 
>  Here is the smallest possible patch fixing the build under 64 bit Unix
> systems after the changes of 75129caf03f5a5e0dd1850c6be4f06a5f231eaf9 which
> resulted in an ambiguity when calling std::min<>() with the arguments of
> type "long int" and "int":

Applied with git-am, and pushed. Thanks.

This change is certainly correct, but the code it improves is nasty.
First, however:

> ---------------------------------- >8 --------------------------------------
>>From 3526d20b7f4c7ba15340981ff21b1e2a1384e2ce Mon Sep 17 00:00:00 2001
> From: Vadim Zeitlin <address@hidden>

Two "From" lines, the first one quoted? I wondered how icedove could have
done that, but it appears that way in the html archives:

http://lists.nongnu.org/archive/html/lmi/2017-03/msg00021.html

I removed the ">From 3526d2" line, saved the rest, and it applied properly.

> Under 64 bit Unix systems the difference between two iterators, which
> has std::ptrdiff_t type, is not of type "int" and hence we need an
> explicit cast to make the call to std::min<>() unambiguous.

Okay, it happened to "work" in the 32-bit world only because there 'int'
and 'ptrdiff_t' are both 32-bit signed integers.

> diff --git a/ledger.cpp b/ledger.cpp
[...]
> @@ -172,7 +172,7 @@ void Ledger::ZeroInforceAfterLapse()
>          }
>      std::vector<double>::iterator b = 
> ledger_invariant_->InforceLives.begin();
>      std::vector<double>::iterator e = ledger_invariant_->InforceLives.end();
> -    b += std::min(e - b, 1 + lapse_year);
> +    b += std::min(static_cast<int>(e - b), 1 + lapse_year);
>      if(b < e)
>          {
>          std::fill(b, e, 0.0);

I couldn't help noticing that, for a vector v, v.end()-v.begin() is
v.size() by definition--as you also observe:

>  Personally, I'd rather use InforceLives.size() and InforceLives.empty()
> here too, but this is much less important than making this code compile in
> the first place.

Okay, now that it compiles, how should we rewrite it for clarity?

Even after all these years, I still think in APL, so my first idea is
  data ← original_length ↑ shorter_length ↑ data
thus:

    ledger_map_t const& l_map_rep = ledger_map_->held();
    using T = decltype(ledger_invariant_->InforceLives)::size_type;
    T original_size = ledger_invariant_->InforceLives.size();
    T lapse_year = T(0);
    for(auto const& i : l_map_rep)
        {
        lapse_year = std::max(lapse_year, static_cast<T>(i.second.LapseYear));
        }
    if(lapse_year < original_size)
        {
        ledger_invariant_->InforceLives.resize(lapse_year);
        ledger_invariant_->InforceLives.resize(original_size);
        }

(I don't see a much less annoying way to get size_type; ptrdiff_t isn't
guaranteed to work forever, and I don't ever want to have to correct
this type again.)

Is that really likely to be far short of optimally efficient? resize()
doesn't reduce the vector's capacity, so there's no memory allocation.
Sure, we're destroying the last (original_size-lapse_year) elements
and default-inserting the same number, but notionally at least that's
no more complicated than explicitly zeroing the elements.