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

[Vadim Zeitlin]

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

...detailed argument about bourn_cast correctness snipped...
GC> I speculate that this argument can be faithfully summarized by saying
GC> that for integer comparison to behave counterintuitively, the types
GC> must be of different signednesses and one must be negative, but there
GC> is no path through the code where that can happen.

 Yes, I agree with your analysis and I had initially done the same one,
albeit much less rigorously, to convince myself that this code is indeed
correct.

 It still somewhat bothers me, however, to see "from < 0" comparison for a
possibly unsigned or not signed type though and, worse, it bothers g++ 6 as
well:

In file included from bourn_cast_test.cpp:24:0:
bourn_cast.hpp: In instantiation of ‘To bourn_cast(From) [with To = bool; From 
= bool]’:
bourn_cast_test.cpp:41:5:   required from ‘void test_same(const char*, int) 
[with T = bool]’
bourn_cast_test.cpp:218:57:   required from here
bourn_cast.hpp:87:65: error: comparison of constant ‘0’ with boolean expression 
is always false [-Werror=bool-compare]
     if(! to_traits::is_signed && from_traits::is_signed && from < 0)
                                                            ~~~~~^~~
cc1plus: all warnings being treated as errors
Makefile:7405: recipe for target 'test_bourn_cast-bourn_cast_test.o' failed

This can, of course, be easily fixed by disabling the warning and the
attached patch does it. If we decide to not do anything else, could you
please apply it to allow me to build the test suite with gcc 6 here?


 But ideally I'd still like to find a way to write all these tests without
comparing types of different signedness (which is a common mistake in C and
C++ and the reason for "-Wsign-compare" existence). This would allow
avoiding disabling the warning and could also make the code even more
obviously correct. Unfortunately I don't think it's going to be simple to
do it in C++11. In C++17 we could write something like this:
---------------------------------- >8 --------------------------------------
template<typename To, typename From>
constexpr
inline To bourn_cast(From from)
{
    using to_traits   = std::numeric_limits<To>;
    using from_traits = std::numeric_limits<From>;
    static_assert(  to_traits::is_specialized);
    static_assert(from_traits::is_specialized);

    if constexpr(from_traits::is_signed)
        {
        if constexpr(to_traits::is_signed)
            {
            if(from < to_traits::lowest())
                throw std::runtime_error("Cast would transgress lower limit.");
            }
        else
            {
            if(from < 0)
                throw std::runtime_error("Cast would convert negative to 
unsigned.");
            }

        if constexpr(from_traits::is_integer)
            {
            if(0 <= from && to_traits::max() < 
static_cast<std::make_unsigned_t<From>>(from))
                throw std::runtime_error("Cast would transgress upper limit.");
            }
        else
            {
            if(to_traits::max() < from)
                throw std::runtime_error("Cast would transgress upper limit.");
            }
        }
    else
        {
        if(to_traits::max() < from)
            throw std::runtime_error("Cast would transgress upper limit.");
        }

    return static_cast<To>(from);
}
---------------------------------- >8 --------------------------------------
(also requires #including <type_traits> to actually compile) and personally
I think I prefer this, in spite of it being longer (partly due to some
redundancy that could be avoided by the use of a helper function), to the
original version. But to translate this in C++11 we'd need several helper
functions and I don't think the result would be especially clear. OTOH we
still would at least be able to avoid disabling the warning, which is nice.
Do you think it would make sense to attempt to translate the approach above
to C++11 or do you dislike its verbosity so much that I shouldn't even
bother with doing this?

 Please let me know,
VZ

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