[cfe-commits] Support <x>-to-bool warnings for more contexts

David Blaikie dblaikie at gmail.com
Wed Apr 18 12:50:31 PDT 2012 

On Wed, Apr 18, 2012 at 12:15 PM, Matthieu Monrocq
<matthieu.monrocq at gmail.com> wrote:
>
>
> Le 18 avril 2012 20:36, David Blaikie <dblaikie at gmail.com> a écrit :
>
>> On Fri, Apr 6, 2012 at 4:42 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> > On Mon, Apr 2, 2012 at 9:44 PM, Nico Weber <thakis at chromium.org> wrote:
>> >> Do you have any numbers on bug / false positive ratios before and
>> >> after this change?
>> >
>> > I'm surprised this didn't catch more - but I found only 2 cases where
>> > this diagnostic fired (on the same function call, no less) & they seem
>> > like perfectly reasonable true positives. Something like:
>> >
>> > void func(bool, bool);
>> > func(0.7, 0.3);
>> >
>> > I'm not really sure what the author intended, but I'm fairly certain
>> > they didn't get it (unless their intent was to confuse future
>> > readers).
>>
>> So this was a little more positive than it looks - these were the new
>> warnings for -Wliteral-conversion that were found by this patch. The
>> new warnings for -Wconstant-conversion (these were the vast majority
>> of the new warnings for my change  - though we don't use
>> -Wnull-conversion at the moment, so I haven't measured the increase in
>> that warning, for example) are a bit more difficult.
>>
>> While a lot of cases were legitimate, there are a few major false
>> positive cases:
>>
>> * in the /existing/ warning, we have a 'false'-ish positive involving
>> code like this: int i = std::string::npos; ... if (i ==
>> std::string::npos) - npos is actually, say, LONG_MAX, so when stored
>> in an int it truncates to -1, but it compares == to -1 just fine.
>> Perhaps we could subcategorize -Wconstant-conversion to allow these
>> particular cases that happen to map back/forth non-destructively?
>>
>
> I have quashed about a hundred of similar bugs this very morning
> (-Wsign-conversion actually, or something similar).
>
> Even though it is fine for now, it is just a bug in waiting as far as I am
> concerned because `int` and `std::string::size_type` vary not only in
> signedness but also, possibly, in bit width. For example, `i >= 0` should be
> tautologically true here since a length is necessarily positive, but because
> of the weird cast it will return false :x
>
> So I don't see much value in delaying the apparition of the warning, on the
> contrary the more we delay it, the more code will have been written with
> this `int` type and the more work it will be to correct the issue.

I can't say I disagree - and this was my line of reasoning at first. I
just thought I might err on the side of caution & see if people felt
that a little more choice about finding lurking/future bugs versus
current bugs might be a tradeoff that Clang users wanted, or whether
this was a clear case of "that's buggy enough that I'm willing to fix
it to get clean on a diagnostic that'll find more significant bugs
too".

Currently -Wconstant-conversion catches more cases like this (int =
string::npos) than anything else in code I'm looking at - it seems
those cases are considered low-pri enough that the warning isn't
turned on. But with my improvement we find lots of good things that
might be enough to justify fixing these lesser issues.

>>
>> * The major case of false positives with my improved warning amounts
>> to a use case like this: #define MY_ALLOC(Type, Count)
>> malloc(sizeof(Type) * ((Count) ? Count : 1)) // the actual code is a
>> bit more involved, but it's in Python's PyMem_NEW macro
>>  The problem is that when you pass a compile-time constant count, now
>> we appear to be truncating an integer (stuffing that big count into
>> zero or one of a boolean). It would be nice if we could somehow detect
>> the case where a macro parameter is used inside a constant expression
>> & flag that constant expression as "not so constant". This logic will
>> be necessary for improvements to Clang's unreachable code diagnostic
>> anyway (we need to know when constant expressions might still vary
>> depending on the build settings (or 'call' sites in the case of
>> macros))
>>  * equally, improvements to allow for sizeof expressions to trigger
>> similar "not quite constant" flags would be good. While "if
>> (sizeof(X))" is silly & we can happily warn on that, "if (sizeof(X) -
>> 3)" might be less clear cut (or sizeof in some other part of a
>> constant expression) - though I haven't seen (m)any false positives
>> like this.
>>
>> * Template parameters - this leads to code a lot like macros:
>> template<int N> void func() { ... if (N) { ... } }; I've currently
>> worked around this by having "IgnoreParenImpCasts" not ignore
>> SubstNonTypeTemplateParmExprs - this is a bit of a dirty hack (both
>> because this code was presumably written this way for a reason -
>> though removing it doesn't regress any test cases - and because I
>> don't think it falls down as soon as N is a subexpression such as "if
>> (N - 3)")
>>
>
> I can only agree with template parameters, I remember such a template:
>
> template <typename T, T Min, T Max>
> class BoundedIntegral {
> public:
>     void set(T t) {
>         if (t < Min or t >= Max) { throw some_exception(t); }
>         _value = t;
>     }
> private:
>     T _value;
> };
>
> When instantiated as   BoundedIntegral<unsigned, 0u, 128u>   is gives a
> warning (under gcc) because the test   t < 0u   is tautologically true...
> Doh!
>
> I just had to disable the warning, which is a pity because it's often a good
> indicator of dubious checks.

Yep - apparently Clang doesn't do this any better:

$ cat taut.cpp
template<typename T>
bool func(T t) {
  return t < 0;
}
int main() {
  return func(0u);
}
$ clang++ taut.cpp
taut.cpp:3:12: warning: comparison of unsigned expression < 0 is
always false [-Wtautological-compare]
  return t < 0;
         ~ ^ ~
taut.cpp:6:10: note: in instantiation of function template
specialization 'func<unsigned int>' requested here
  return func(0u);
         ^

I'm not sure whether the changes necessary to improve
-Wconstant-conversion would also be relevant to fixing this similar
case in -Wtautological-compare, but they might. I suspect the easier
thing is just to perform -Wtautological-compare on template patterns,
not template instantiations.

- David

>
> -- Matthieu
>
>>
>> Any thoughts on whether or not these are reasonable goals and how best
>> to achieve them would be most welcome,
>>
>> - David
>>
>> >
>> > - David
>> >
>> >> On Mon, Apr 2, 2012 at 6:03 PM, David Blaikie <dblaikie at gmail.com>
>> >> wrote:
>> >>> SemaChecking.cpp:3989 currently returns early from checking implicit
>> >>> conversions after it tests some specific X-to-boolean cases (including
>> >>> string and funciton literals) but before checking various other cases
>> >>> later on like NULL-to-X and wide integer literal to narrow integer.
>> >>>
>> >>> This change removes the early return, fixes the diagnostic (to
>> >>> correctly emit the fact that non-zero literals produce a "true"
>> >>> boolean value rather than simply truncating the larger integer
>> >>> literal), and updates the tests. In some cases the test cases were
>> >>> fixed or updated (//expected-warning), in others I simply suppressed
>> >>> the diagnostic because there adding the expected-warnings would've
>> >>> added a lot of noise to the test cases*.
>> >>>
>> >>> * This last case is a little bit questionable: in one specific case we
>> >>> produce a really good diagnostic about constant integer literals used
>> >>> in boolean contexts:
>> >>>
>> >>> int f1();
>> >>> bool f2() {
>> >>>  return f1() && 42;
>> >>> }
>> >>>
>> >>> we produce:
>> >>>
>> >>> conv.cpp:3:15: warning: use of logical '&&' with constant operand
>> >>> [-Wconstant-logical-operand]
>> >>>  return f1() && 42;
>> >>>              ^  ~~
>> >>> conv.cpp:3:15: note: use '&' for a bitwise operation
>> >>>  return f1() && 42;
>> >>>              ^~
>> >>>              &
>> >>> conv.cpp:3:15: note: remove constant to silence this warning
>> >>>  return f1() && 42;
>> >>>             ~^~~~~
>> >>>
>> >>> But then with my patch we get an extra diagnostic after the above
>> >>> warning/notes:
>> >>>
>> >>> conv.cpp:3:18: warning: implicit conversion from 'int' to 'bool'
>> >>> changes value from 42 to true [-Wconstant-conversion]
>> >>>  return f1() && 42;
>> >>>              ~~ ^~
>> >>>
>> >>> which isn't great - since we already gave a much more specific
>> >>> diagnosis of the problem in the first warning. If there's some nice
>> >>> way that we could suppress the second one whenever the first one is
>> >>> provided (unless the first one is only a warning and the second is
>> >>> -Werror'd?) I'd be happy to implement that.
>> >>>
>> >>>
>> >>>
>> >>> Another thing I noticed as I was exploring this. We have a warning for
>> >>> float-literal-to-int such as:
>> >>>
>> >>> conv.cpp:2:9: warning: implicit conversion turns literal
>> >>> floating-point number into integer: 'double' to 'int'
>> >>> [-Wliteral-conversion]
>> >>> int i = 3.1415;
>> >>>    ~   ^~~~~~
>> >>>
>> >>> But this warning is off-by-default. Why is that? It's already
>> >>> relatively conservative (allowing things like : "int i = 3.0" because
>> >>> 3.0 converts to an int without loss of precision) - though it's not a
>> >>> DiagnoseRuntimeBehavior, which it could be changed to (to be
>> >>> consistent with similar things for integers like "unsigned char c =
>> >>> 256").
>> >>>
>> >>> Or is it really that common to deliberately use floating point
>> >>> literals to initialize integer values?
>> >>>
>> >>> _______________________________________________
>> >>> cfe-commits mailing list
>> >>> cfe-commits at cs.uiuc.edu
>> >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>

More information about the cfe-commits mailing list