Re: [lmi] Integer overflow warnings in bourn_cast with clang

[Vadim Zeitlin]

On Thu, 6 Apr 2017 15:24:08 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2017-04-06 00:24, Vadim Zeitlin wrote:
GC> > 
GC> >  I can confirm that bourn_cast test compiles and passes with g++ 6.3 as
GC> > well as with 4.9 normally used for testing. Unfortunately it does not
GC> > compile with clang 4.0, giving several errors:
GC> > 
GC> > In file included from bourn_cast_test.cpp:49:
GC> > bourn_cast.hpp:134:35: error: overflow in expression; result is 
-9223372036854775808 with type 'long long' [-Werror,-Winteger-overflow]
GC> >         if(From(to_traits::max()) + 1 <= from)
GC> >                                   ^
GC> > bourn_cast.hpp:134:35: error: overflow in expression; result is 
-2147483648 with type 'int' [-Werror,-Winteger-overflow]
GC> 
GC> This is in a block of code that's used only for converting from floating
GC> to integral types, so that 'From' and 'from' are floating. Unfortunately...
GC> 
GC> >  Of course, all of those are actually different instances of the same 
error
GC> > (notice that it happens even for e.g. bourn_cast<int,int>() because the
GC> > code in this line 134 is still compiled even if it's not being executed).
GC> 
GC> ...when 'From' is integral, clang still compiles it even though it's
GC> unreachable. I think the solution is to decompose it into four subfunction
GC> templates, and dispatch to one of the four depending on the template
GC> parameters. I was planning to do that anyway for readability, but now it
GC> seems to be the most direct way to solve an actual problem.

 Yes, this was my next idea if, as I suspected, my too-simple-to-be-correct
proposed change wasn't right.

GC> >  The trivial patch
GC> [...]
GC> > -        if(From(to_traits::max()) + 1 <= from)
GC> > +        if(From(to_traits::max()) <= from - 1)
GC> [...]
GC> > fixes it while allowing the tests to still pass,
GC> 
GC> In what sense do the tests pass?

 I wasn't being facetious, they do pass for me (with my clang-specific
patches and this one on top of b777985b7b0102f665d1d451508d4dc7aabcb76a).
I.e. the last line of output is "!!!! no errors detected" and all 540 tests
succeed. But this is because I'm running tests under x86_64 Linux while
you're probably testing under 32 bit MSW -- and I do see these failures
there (I also see 539 and not 540). So this is clearly because int64_t can
represent something not representable in an int32_t.

GC> > but I'm not sure at all if
GC> > this doesn't introduce some other problem, I'm afraid this code has become
GC> > too complicated for me to reason about it with certainty.
GC> 
GC> > -        if(From(to_traits::max()) + 1 <= from)
GC> 
GC> Here, the integral maximum is 2^N - 1, and adding one makes it 2^N.
GC> If the floating ("From") type has enough mantissa bits to represent the
GC> integral type's maximum, then "+ 1" really does add one. Otherwise, "+ 1"
GC> does nothing, but that's okay because "From(to_traits::max())" was already
GC> rounded to nearest (if IEC 559 is respected), i.e. to 2^N. Either way, the
GC> LHS equals 2^N.
GC> 
GC> > +        if(From(to_traits::max()) <= from - 1)
GC> 
GC> This is not equivalent. If we were to follow boost's design decision
GC> and accept truncating conversions from floating to integral, we'd get
GC> incorrect results.

 Would we though? AFAICS we'd still refuse to cast, but just with a "would
not preserve value" error instead of "would transgress upper limit" one.
So, IOW, maybe this check, and more precise error message, could be just
removed completely?

 The solution with different template functions is better in theory, but
it's also more complex and I'm not sure it really has any advantages in
practice. Any error messages produced due to a failure in this cast will
almost certainly be completely incomprehensible to any end user and,
hopefully, relatively clear to any developer if they include the value
being cast and the types being casted from/to (and not really useful
without them).

 Regards,
VZ