lmi
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Greg Chicares
Subject: Re: [lmi] [PATCH] Small fix for "unsafe comparison" warning in MSVS build
Date: Tue, 10 Jul 2018 20:31:20 +0000
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0

On 2018-07-02 23:37, Vadim Zeitlin wrote:
> 
>  I've just created https://github.com/vadz/lmi/pull/87 which avoids getting
> a warning about unsafe comparison of a bool with a double when building
> with MSVS. The warning is useful and I'm actually rather surprised that
> neither gcc nor clang give it,

gcc has
  -Wbool-compare
which warns about comparisons between bool and other integral types
(but not between bool and floating types); and
  -Wfloat-equal
which seems unusable (it might not even allow '!= 0.0' in your patch).

> so I don't want to disable it and I think
> the PR above is the best (although not the only one, as explained in the
> commit message) way to fix it.

The commit message says, in part:

| Alternatively, the type of individual_selection_ could be changed to
| "double", but it seems better to keep it as boolean, as this is what it
| really is.

In commit 6fa7f99
  Resolve more '-Wconversion' issues
it changed from int to bool:

-    int individual_selection_ {99};
+    bool individual_selection_ {}; // Initialized by add_ledger().

and I'm sure you're right that 'bool' is the proper type.

>  Please let me know what do you think,

First of all, are you certain that this warning arises only in this sole
instance, and in no other case with a complete build from scratch? I.e.:
  make clean && make all && make unit_tests
I strongly prefer to find and fix all occurrences OAOO instead of trying to
fix them piecemeal as we happen to notice them.

Even if this really is the only occurrence of this warning, the code seems
perfectly clear and correct to me, so can the warning for comparing floating
to boolean be disabled selectively by refining the set of warnings selected
on the command line, rather like our use of
  -Wconversion -Wno-sign-conversion
for g++?

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]