Re: [lmi] [PATCH] Small fix for "unsafe comparison" warning in MSVS build

[Vadim Zeitlin]

On Tue, 10 Jul 2018 20:31:20 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2018-07-02 23:37, Vadim Zeitlin wrote:
GC> > 
GC> >  I've just created https://github.com/vadz/lmi/pull/87 which avoids 
getting
GC> > a warning about unsafe comparison of a bool with a double when building
GC> > with MSVS. The warning is useful and I'm actually rather surprised that
GC> > neither gcc nor clang give it,
GC> 
GC> gcc has
GC>   -Wbool-compare
GC> which warns about comparisons between bool and other integral types
GC> (but not between bool and floating types);

 Unfortunately I don't think it even warns about comparison with other
integral types, only with integral constants. But it's true that it doesn't
warn about comparisons with floating point constants in any case:

        % echo 'bool foo(bool x, int y) { return x == 0.1; }'|g++-8 -Wall 
-Wbool-compare -fsyntax-only -x c++ -

produces no warnings.


GC> First of all, are you certain that this warning arises only in this sole
GC> instance, and in no other case with a complete build from scratch?

 I don't build all the code with MSVS, i.e. I only build a couple of tests.
But in the targets I do build, it is indeed the only instance of this (or
any other) warning, which is why I'd like to get rid of it to have clean
build again.

GC> Even if this really is the only occurrence of this warning, the code seems
GC> perfectly clear and correct to me,

 I'm surprised. Assigning a double to a boolean, as the current code does,
is a huge red flag for me. Do you really condone such type-unsafeness?

GC> so can the warning for comparing floating to boolean be disabled
GC> selectively by refining the set of warnings selected on the command
GC> line, rather like our use of
GC>   -Wconversion -Wno-sign-conversion
GC> for g++?

 It could be disabled on the command line, but this would be global (or
per-file, but doing it per-file would make the build slightly slower as it
wouldn't compile this file in parallel with the other ones, as it'd need
different compiler flags). I'd prefer not to do it if possible.

GC> Otherwise...the introduction of an extra variable in the patch just looks
GC> like a missed opportunity to write tersely IMO, so can we just fix it in
GC> the most direct way, i.e.:
GC> -        if(invar.GroupIndivSelection != individual_selection_)
GC> +        // static_cast to avoid an msvc warning:
GC> +        if(static_cast<bool>(invar.GroupIndivSelection) != 
individual_selection_)
GC> ? 

 Yes, this would work too. IMHO it's preferable to avoid the cast, but if
you think it's better like this, I don't really mind.

 Thanks,
VZ