<br><br><div class="gmail_quote">Le 18 avril 2012 20:36, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>></span> a écrit :<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Fri, Apr 6, 2012 at 4:42 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> On Mon, Apr 2, 2012 at 9:44 PM, Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> wrote:<br>
>> Do you have any numbers on bug / false positive ratios before and<br>
>> after this change?<br>
><br>
> I'm surprised this didn't catch more - but I found only 2 cases where<br>
> this diagnostic fired (on the same function call, no less) & they seem<br>
> like perfectly reasonable true positives. Something like:<br>
><br>
> void func(bool, bool);<br>
> func(0.7, 0.3);<br>
><br>
> I'm not really sure what the author intended, but I'm fairly certain<br>
> they didn't get it (unless their intent was to confuse future<br>
> readers).<br>
<br>
So this was a little more positive than it looks - these were the new<br>
warnings for -Wliteral-conversion that were found by this patch. The<br>
new warnings for -Wconstant-conversion (these were the vast majority<br>
of the new warnings for my change - though we don't use<br>
-Wnull-conversion at the moment, so I haven't measured the increase in<br>
that warning, for example) are a bit more difficult.<br>
<br>
While a lot of cases were legitimate, there are a few major false<br>
positive cases:<br>
<br>
* in the /existing/ warning, we have a 'false'-ish positive involving<br>
code like this: int i = std::string::npos; ... if (i ==<br>
std::string::npos) - npos is actually, say, LONG_MAX, so when stored<br>
in an int it truncates to -1, but it compares == to -1 just fine.<br>
Perhaps we could subcategorize -Wconstant-conversion to allow these<br>
particular cases that happen to map back/forth non-destructively?<br>
<br></blockquote><div><br>I have quashed about a hundred of similar bugs this very morning (-Wsign-conversion actually, or something similar).<br><br>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<br>
<br>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.<br>
<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
* The major case of false positives with my improved warning amounts<br>
to a use case like this: #define MY_ALLOC(Type, Count)<br>
malloc(sizeof(Type) * ((Count) ? Count : 1)) // the actual code is a<br>
bit more involved, but it's in Python's PyMem_NEW macro<br>
The problem is that when you pass a compile-time constant count, now<br>
we appear to be truncating an integer (stuffing that big count into<br>
zero or one of a boolean). It would be nice if we could somehow detect<br>
the case where a macro parameter is used inside a constant expression<br>
& flag that constant expression as "not so constant". This logic will<br>
be necessary for improvements to Clang's unreachable code diagnostic<br>
anyway (we need to know when constant expressions might still vary<br>
depending on the build settings (or 'call' sites in the case of<br>
macros))<br>
* equally, improvements to allow for sizeof expressions to trigger<br>
similar "not quite constant" flags would be good. While "if<br>
(sizeof(X))" is silly & we can happily warn on that, "if (sizeof(X) -<br>
3)" might be less clear cut (or sizeof in some other part of a<br>
constant expression) - though I haven't seen (m)any false positives<br>
like this.<br>
<br>
* Template parameters - this leads to code a lot like macros:<br>
template<int N> void func() { ... if (N) { ... } }; I've currently<br>
worked around this by having "IgnoreParenImpCasts" not ignore<br>
SubstNonTypeTemplateParmExprs - this is a bit of a dirty hack (both<br>
because this code was presumably written this way for a reason -<br>
though removing it doesn't regress any test cases - and because I<br>
don't think it falls down as soon as N is a subexpression such as "if<br>
(N - 3)")<br>
<br></blockquote><div><br>I can only agree with template parameters, I remember such a template:<br><br>template <typename T, T Min, T Max><br>class BoundedIntegral {<br>public:<br> void set(T t) {<br> if (t < Min or t >= Max) { throw some_exception(t); }<br>
_value = t;<br> }<br>private:<br> T _value;<br>};<br><br>When instantiated as BoundedIntegral<unsigned, 0u, 128u> is gives a warning (under gcc) because the test t < 0u is tautologically true... Doh!<br>
<br>I just had to disable the warning, which is a pity because it's often a good indicator of dubious checks.<br><br>-- Matthieu<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Any thoughts on whether or not these are reasonable goals and how best<br>
to achieve them would be most welcome,<br>
<br>
- David<br>
<br>
><br>
> - David<br>
><br>
>> On Mon, Apr 2, 2012 at 6:03 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>>> SemaChecking.cpp:3989 currently returns early from checking implicit<br>
>>> conversions after it tests some specific X-to-boolean cases (including<br>
>>> string and funciton literals) but before checking various other cases<br>
>>> later on like NULL-to-X and wide integer literal to narrow integer.<br>
>>><br>
>>> This change removes the early return, fixes the diagnostic (to<br>
>>> correctly emit the fact that non-zero literals produce a "true"<br>
>>> boolean value rather than simply truncating the larger integer<br>
>>> literal), and updates the tests. In some cases the test cases were<br>
>>> fixed or updated (//expected-warning), in others I simply suppressed<br>
>>> the diagnostic because there adding the expected-warnings would've<br>
>>> added a lot of noise to the test cases*.<br>
>>><br>
>>> * This last case is a little bit questionable: in one specific case we<br>
>>> produce a really good diagnostic about constant integer literals used<br>
>>> in boolean contexts:<br>
>>><br>
>>> int f1();<br>
>>> bool f2() {<br>
>>> return f1() && 42;<br>
>>> }<br>
>>><br>
>>> we produce:<br>
>>><br>
>>> conv.cpp:3:15: warning: use of logical '&&' with constant operand<br>
>>> [-Wconstant-logical-operand]<br>
>>> return f1() && 42;<br>
>>> ^ ~~<br>
>>> conv.cpp:3:15: note: use '&' for a bitwise operation<br>
>>> return f1() && 42;<br>
>>> ^~<br>
>>> &<br>
>>> conv.cpp:3:15: note: remove constant to silence this warning<br>
>>> return f1() && 42;<br>
>>> ~^~~~~<br>
>>><br>
>>> But then with my patch we get an extra diagnostic after the above warning/notes:<br>
>>><br>
>>> conv.cpp:3:18: warning: implicit conversion from 'int' to 'bool'<br>
>>> changes value from 42 to true [-Wconstant-conversion]<br>
>>> return f1() && 42;<br>
>>> ~~ ^~<br>
>>><br>
>>> which isn't great - since we already gave a much more specific<br>
>>> diagnosis of the problem in the first warning. If there's some nice<br>
>>> way that we could suppress the second one whenever the first one is<br>
>>> provided (unless the first one is only a warning and the second is<br>
>>> -Werror'd?) I'd be happy to implement that.<br>
>>><br>
>>><br>
>>><br>
>>> Another thing I noticed as I was exploring this. We have a warning for<br>
>>> float-literal-to-int such as:<br>
>>><br>
>>> conv.cpp:2:9: warning: implicit conversion turns literal<br>
>>> floating-point number into integer: 'double' to 'int'<br>
>>> [-Wliteral-conversion]<br>
>>> int i = 3.1415;<br>
>>> ~ ^~~~~~<br>
>>><br>
>>> But this warning is off-by-default. Why is that? It's already<br>
>>> relatively conservative (allowing things like : "int i = 3.0" because<br>
>>> 3.0 converts to an int without loss of precision) - though it's not a<br>
>>> DiagnoseRuntimeBehavior, which it could be changed to (to be<br>
>>> consistent with similar things for integers like "unsigned char c =<br>
>>> 256").<br>
>>><br>
>>> Or is it really that common to deliberately use floating point<br>
>>> literals to initialize integer values?<br>
>>><br>
>>> _______________________________________________<br>
>>> cfe-commits mailing list<br>
>>> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>>><br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br>