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

[Greg Chicares]

On 2017-04-06 00:24, Vadim Zeitlin wrote:
> 
>  I can confirm that bourn_cast test compiles and passes with g++ 6.3 as
> well as with 4.9 normally used for testing. Unfortunately it does not
> compile with clang 4.0, giving several errors:
> 
> In file included from bourn_cast_test.cpp:49:
> bourn_cast.hpp:134:35: error: overflow in expression; result is 
> -9223372036854775808 with type 'long long' [-Werror,-Winteger-overflow]
>         if(From(to_traits::max()) + 1 <= from)
>                                   ^
> bourn_cast.hpp:134:35: error: overflow in expression; result is -2147483648 
> with type 'int' [-Werror,-Winteger-overflow]

This is in a block of code that's used only for converting from floating
to integral types, so that 'From' and 'from' are floating. Unfortunately...

>  Of course, all of those are actually different instances of the same error
> (notice that it happens even for e.g. bourn_cast<int,int>() because the
> code in this line 134 is still compiled even if it's not being executed).

...when 'From' is integral, clang still compiles it even though it's
unreachable. I think the solution is to decompose it into four subfunction
templates, and dispatch to one of the four depending on the template
parameters. I was planning to do that anyway for readability, but now it
seems to be the most direct way to solve an actual problem.

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

In what sense do the tests pass? With this patch applied, gcc says:

???? 10 test errors detected; 529 tests succeeded

and all ten errors are of this nature:

???? test failed: Caught exception
    'Cast would not preserve value.'
  when
    'Cast would transgress upper limit.'
  was expected.

With clang, did you see the same results and reason that the exact
identity of the exception doesn't matter as long as some exception
is thrown? Or did you get a clean run with zero errors detected?

> but I'm not sure at all if
> this doesn't introduce some other problem, I'm afraid this code has become
> too complicated for me to reason about it with certainty.

> -        if(From(to_traits::max()) + 1 <= from)

Here, the integral maximum is 2^N - 1, and adding one makes it 2^N.
If the floating ("From") type has enough mantissa bits to represent the
integral type's maximum, then "+ 1" really does add one. Otherwise, "+ 1"
does nothing, but that's okay because "From(to_traits::max())" was already
rounded to nearest (if IEC 559 is respected), i.e. to 2^N. Either way, the
LHS equals 2^N.

> +        if(From(to_traits::max()) <= from - 1)

This is not equivalent. If we were to follow boost's design decision
and accept truncating conversions from floating to integral, we'd get
incorrect results.

>  Still, it does seem wrong to add 1 to the maximally representable value of
> type "To" without being certain that it is _strictly_ less than that of
> type "From".

Good point. I'll see whether the gcc version I'm using has __int128.