<div dir="ltr">Sean - here's the thread on that old patch (that might've caught Kostya's "bool NumFoo = 10" case), in case you're interested.</div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, May 15, 2012 at 9:59 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Committed some of the easier parts of this separately in r156826 -<br>
providing (floating) literal-to-bool and NULL-to-bool, but not<br>
exposing the problems with constant-to-bool until I can iron out the<br>
false positives.<br>
<div class="HOEnZb"><div class="h5"><br>
On Mon, Apr 23, 2012 at 4:35 PM, Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> wrote:<br>
> On Sat, Apr 21, 2012 at 9:54 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>> On Sat, Apr 21, 2012 at 9:16 PM, Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> wrote:<br>
>>> I gave this a try in chrome. Here's two cases where this warns on that<br>
>>> make me doubtful of this patch.<br>
>><br>
>> I agree in its current state it'll need some tweaking to improve the<br>
>> accuracy of the cases it opens up. Or are you saying you think it's<br>
>> non-viable on principle/beyond correction?<br>
><br>
> I was just commenting on the patch as-is.<br>
><br>
>><br>
>>> 1.) It warns on code like<br>
>>><br>
>>> while(YY_CURRENT_BUFFER){ ... }<br>
>>><br>
>>> where YY_CURRENT_BUFFER is something that's defined flex:<br>
>>><br>
>>> ./compiler/glslang_lex.cpp:2878:8: warning: implicit conversion of<br>
>>> NULL constant to 'bool' [-Wnull-conversion]<br>
>>> while(YY_CURRENT_BUFFER){<br>
>>> ^~~~~~~~~~~~~~~~~<br>
>>> ./compiler/glslang_lex.cpp:307:29: note: expanded from macro 'YY_CURRENT_BUFFER'<br>
>>> : NULL)<br>
>>><br>
>>> If you use flex, you have to turn off Wnull-conversion because of this<br>
>>> issue. Before the patch, Wnull-conversion was a useful warning.<br>
>><br>
>> Hmm - wonder what the right fix for this is...<br>
>><br>
>> I wouldn't mind seeing the full definition of YY_CURRENT_BUFFER if you<br>
>> have a chance to send it to me. It /sounds/ like the conditional<br>
>> operator being used there isn't doing what the author thinks it's<br>
>> doing (it's probably got a bool argument on the LHS & so the NULL on<br>
>> the rhs is always being converted to 'false' & should just be written<br>
>> that way).<br>
>><br>
>>> 2.) It warns on this:<br>
>>><br>
>>> ../../third_party/skia/src/core/SkScalerContext.cpp:380:9: warning:<br>
>>> implicit conversion from 'int' to 'bool' changes value from 160 to<br>
>>> true [-Wconstant-conversion]<br>
>>> if (SK_FREETYPE_LCD_LERP) {<br>
>>> ^~~~~~~~~~~~~~~~~~~~<br>
>>> ../../third_party/skia/src/core/SkScalerContext.cpp:372:33: note:<br>
>>> expanded from macro 'SK_FREETYPE_LCD_LERP'<br>
>>> #define SK_FREETYPE_LCD_LERP 160<br>
>>> ^~~<br>
>>><br>
>>> This is fairly common in code.<br>
>><br>
>> Yep - my thinking was that we could reduce the -Wconstant-conversion<br>
>> cases that convert to bool could be limited to literals rather than<br>
>> arbitrary expressions (though we'd have to skip the macro/constant<br>
>> cases too - but that might miss a lot of really good cases... )<br>
>><br>
>>> (The warning did find a few cases where we're saying 'return NULL' but<br>
>>> should be saying 'return false', but nothing interesting.<br>
>><br>
>> Curious - given all the fun things I found I'm surprised it didn't hit<br>
>> other fun things in chromium. Thanks for giving it a go, though.<br>
>><br>
>>> I didn't do<br>
>>> a full build of chrome because the build died fairly quickly due to<br>
>>> visibility issues caused by one of espindola's recent patches, so I<br>
>>> tracked that down instead.)<br>
>><br>
>> Fair enough,<br>
>> - David<br>
>><br>
>>><br>
>>> Nico<br>
>>><br>
>>> On Wed, Apr 18, 2012 at 3:42 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>>>> On Wed, Apr 18, 2012 at 3:04 PM, Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> wrote:<br>
>>>>> On Wed, Apr 18, 2012 at 11:36 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>>>>>> 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>
>>>>> This sounds to me like "more trouble than it's worth". Did you find<br>
>>>>> any interesting bugs with this?<br>
>>>><br>
>>>> Quite a few, yes. Here's a smattering of examples:<br>
>>>><br>
>>>> enum X { A, B, COUNT };<br>
>>>> std::vector<bool> b(true, COUNT);<br>
>>>><br>
>>>> x &= !flag; // in xerces, actually<br>
>>>><br>
>>>> void log_if(int severity, bool condition);<br>
>>>> log_if(condition, 3);<br>
>>>><br>
>>>> bool func() { ... return ERROR_CODE_FOO; } // various kinds of error<br>
>>>> codes, often enums<br>
>>>><br>
>>>> bool b;<br>
>>>> int i;<br>
>>>> ...<br>
>>>> b = 10; // user seems to have jumbled up the variables, or their types<br>
>>>> i = true;<br>
>>>> // similar mistakes to this, except with function calls<br>
>>>> ("set_new_uid(5)" when the flag was really about whether a new uid is<br>
>>>> created, not specifying the uid value itself)<br>
>>>> // a lot of these, admittedly, come up in test code where more<br>
>>>> constants are used - though I'm not sure how much better that makes me<br>
>>>> feel about them<br>
>>>><br>
>>>> void func(int);<br>
>>>> func(FLAG1 || FLAG2); // should be FLAG1 | FLAG2<br>
>>>><br>
>>>> if (FLAG1 || FLAG2) // should be "(x == FLAG1 || x == FLAG2)"<br>
>>>><br>
>>>> bool maxThings = INT_MAX; // fairly clear mistake in the declaration<br>
>>>> of this type<br>
>>>> void func(int);<br>
>>>> func(maxThings);<br>
>>>><br>
>>>> (x & !(sizeof(void*) - 1)) // probably meant '~' not '!', I believe<br>
>>>><br>
>>>> if (0 == x && FLAG) // similar to previous examples<br>
>>>><br>
>>>> bool status;<br>
>>>> ...<br>
>>>> status = -4; // yay, random constants!<br>
>>>><br>
>>>> while (1729) // I've no idea what this person had in mind... but OK,<br>
>>>> probably working as they intended<br>
>>>><br>
>>>> if (some_const % other_const) // false positive<br>
>>>><br>
>>>> bool func() {<br>
>>>> ...<br>
>>>> return 0;<br>
>>>> ...<br>
>>>> return 1;<br>
>>>> ...<br>
>>>> return 2; // aha! :/<br>
>>>> }<br>
>>>><br>
>>>> Well, that's a rough sample - the enum flag kind of cases seem pretty<br>
>>>> common, or just passing literals of the wrong type to functions or<br>
>>>> constructors (sometimes not as literals, but as constants defined<br>
>>>> elsewhere).<br>
>>>><br>
>>>>><br>
>>>>> Nico<br>
>>>>><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>
>>>>>> * 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>
>>>>>> 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>
</div></div></blockquote></div><br></div>