Re: [lmi] Is bourn_cast demonstrably correct? (+PATCH)

[Vadim Zeitlin]

On Mon, 20 Mar 2017 20:04:03 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2017-03-20 18:44, Vadim Zeitlin wrote:
...
GC> > and, worse, it bothers g++ 6 as well:
GC> > 
GC> > bourn_cast.hpp:87:65: error: comparison of constant ‘0’ with boolean 
expression is always false [-Werror=bool-compare]
GC> 
GC> ...I didn't get that warning here, and of course we don't want it anywhere.
GC> 
GC> > This can, of course, be easily fixed by disabling the warning and the
GC> > attached patch does it. If we decide to not do anything else, could you
GC> > please apply it to allow me to build the test suite with gcc 6 here?
GC> 
GC> Absolutely. If gcc-4.9.2 complains that it doesn't know that warning,
GC> I'll conditionalize it.

 Yes, it would complain, the warning is new in gcc 5 AFAICT, at least I see
it in the change log for gcc 5.2. And I knew it and even did it correctly,
but somehow attached the wrong patch to my last message (see
http://lists.nongnu.org/archive/html/lmi/2017-03/msg00110.html), sorry.
Please find the correct/up-to-date version attached.

GC> > Unfortunately I don't think it's going to be simple to
GC> > do it in C++11. In C++17 we could write something like this:
GC> [...]
GC> >     if constexpr(from_traits::is_signed)
GC> >         {
GC> >         if constexpr(to_traits::is_signed)
GC> >             {
GC> >             if(from < to_traits::lowest())
GC> >                 throw std::runtime_error("Cast would transgress lower 
limit.");
GC> >             }
GC> >         else
GC> >             {
GC> >             if(from < 0)
GC> >                 throw std::runtime_error("Cast would convert negative to 
unsigned.");
GC> >             }
GC> 
GC> Recently I posted an experimental patch that I think did something
GC> similar:
GC>   http://lists.nongnu.org/archive/html/lmi/2017-03/msg00101.html
GC> +    static constexpr To lower_bound = to_traits::lowest();
GC> +    static constexpr To upper_bound = to_traits::max();
GC> +    static constexpr bool must_test_lower = from_traits::lowest() < 
lower_bound;
GC> +    static constexpr bool must_test_upper = upper_bound < 
from_traits::max();
GC> -    if(! to_traits::is_signed && from < 0)
GC> +    if(to_is_unsigned && from_is_signed && from < 0)
GC>          throw std::runtime_error("Cast would convert negative to 
unsigned.");
GC> 
GC> Would that technique work as well (without warnings) as your C++17
GC> suggestion above?

 No, "from < 0" would still be compiled. Without C++17 constexpr if, we
need to put this comparison in a separate function and use the usual SFINAE
tricks to avoid even compiling it when it's tautologically true.

GC> Are helper functions strictly necessary, or would helper type-aliases
GC> and helper-variables be enough? I ask because I suspect they'd be
GC> easier to read than functions.

 I'm not sure, I'd really have to try to write it like this and see how it
goes and I didn't have time to do it yet (but I wanted to send this reply
quickly just because of the updated patch).


GC> BTW, let me share my short-term plans for this facility. First, I asked
GC> myself whether it should just forbid floating point; the answer is no,
GC> because we really do have code that converts double to integral types
GC> using value_cast<>(), and the whole point of bourn_cast<>() is to
GC> replace boost::numeric_cast<>() in value_cast<>().

 Is the code doing this really supposed to lose precision? I.e. should it
convert M_PI to 3? Or is it only meant to convert 3.0 to 3 and throw for
the numbers with non zero fractional part?

GC> I tried figuring out how to make it strictly correct even if is_iec_559
GC> is false, but that way lies madness,

 And it's also just not very useful nowadays. And if this still doesn't
convince you to abandon this idea, please notice that lmi already wouldn't
compile on a non-IEC-559 architecture because of a static assert to the
contrary in rate_table.cpp.

GC> and I don't have a VAX to test it on anyway.

 Sadly, I can't help you here.
VZ

0001-Also-disable-gcc-Wbool-compare-warning-in-bourn_cast.patch
Description: Text document