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

[David Blaikie]

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).

- 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
>>