Re: [lmi] MSVC warnings for comparisons in bourn_cast

[Greg Chicares]

On 2017-04-25 13:54, Vadim Zeitlin wrote:
>
>  Compiling 2f1ceccc32f0faa5a78045d7ae2b182f01f1ce9f with MSVC I get many
> warnings about comparisons in bourn_cast.hpp, here is just a small but,
> I think, representative sample of them:

[condensed]

> bourn_cast.hpp(231): warning C4018: '<': signed/unsigned mismatch
> bourn_cast.hpp(233): warning C4018: '<': signed/unsigned mismatch
> bourn_cast.hpp(231): warning C4804: '<': unsafe use of type 'bool' in 
> operation
> bourn_cast.hpp(233): warning C4804: '<': unsafe use of type 'bool' in 
> operation
> 
> In all I get many hundreds line of output because the same warnings are
> given for many different source files.

Source code:

231    if(from_traits::is_signed && from < to_traits::lowest())

233    if(to_traits::max() < from)

>  As always, I could disable these warnings globally, but I'd be very
> reluctant to do it because both of them are generally useful. I also

Agreed. We want to enable those warnings everywhere but here.

> probably could (but I didn't test it yet) disable the warnings just in this
> code using pragmas, as it's already done for gcc, which seems to be the
> best approach, but would require having MSVC-specific pragmas in this
> header and so I wanted to ask you about it before making a patch. But I

That sounds best. We already do this for gcc:

#pragma GCC diagnostic ignored "-Wsign-compare"
#pragma GCC diagnostic ignored "-Wbool-compare"

so it would seem natural to do likewise for msvc:

#if defined LMI_MSC
#   pragma warning(disable : 4018)
#   pragma warning(disable : 4804)
#endif // defined LMI_MSC

> also wonder if we couldn't avoid the warnings in the first place. I'm not
> brave enough to even try proposing the solution for the first one
> (signed/unsigned mismatch), but it looks like we could avoid at least the
> second one by using a specialization of these comparisons for bool. Would
> it be worth doing this?

I don't think so:
  - it would make the code more complicated
  - it would solve the bool problem only, but not the signed/unsigned one
  - trying to solve both those problems would probably produce a
    combinatorial explosion (if it's even possible)

The hopeful goal of bourn_cast<>() is to avoid these warnings in all other
code--and, what is far more important, to avoid UB in numeric conversions
whether a compiler warns about it or doesn't. (After all, 'boost/cast.hpp'
does have undefined behavior, and apparently no compiler has identified it.)
IOW, we sequester mixed-mode arithmetic and signed-unsigned conversions in
this "library", analyzing and testing its code carefully; then we use the
library everywhere else to avoid potentially unsafe conversions.

Do all the hundreds of lines of downstream warnings stem from just these
two lines (231 and 233), or from just this integral-to-integral section of
'bourn_cast.hpp'? If so, then I think we should rely on analysis such as
the thread "Is bourn_cast demonstrably correct?" beginning here:
  http://lists.nongnu.org/archive/html/lmi/2017-03/msg00109.html
We can of course reopen that thread to discuss any potential problem that
concerns you, or add more unit tests for these cases. But one of the chief
glories of Henney's work here is its terse simplicity, which makes it
humanly possible to analyze the code exhaustively--so I fear that making
it more complicated by adding special cases to avoid mechanical compiler
warnings would also make it impossible to analyze as carefully, and might
easily make the code less correct.