On Tue, Oct 16, 2012 at 3:34 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank" class="cremed">rtrieu@google.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Changed the fix-it to suggest parentheses around the comparison instead of switching the comparison operator.</blockquote>
<div><br></div><div>I don't think that's the correct fix in all cases. I'd rather detect when either of the candidate operands are floating point, and condition which fixit we display on that.</div><div><br></div>
<div>Additionally, we should test whether either operand is a class type and lacking a definition of the inverted condition, or whether we are inside of the definition of the inverted condition.</div><div><br></div><div>The reason I prefer this is that I'd like to have a narrow list of exclusions to the less verbose fixit hint. When these are integers, it seems like our fixit is going to be less useful if it will only insert parentheses.</div>
<div><br></div><div>-Chandler</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><br></div><div><div>logical-not.cc:2:7: warning: logical not is only applied to the left hand side</div>
<div>
of this comparison [-Wlogical-not-compare]</div><div> if (!5 < 4)</div><div> ^ ~</div><div>logical-not.cc:2:7: note: add parentheses after the '!' to evaluate the</div><div> condition first</div>
<div> if (!5 < 4)</div><div> ^</div><div> ( )</div><div>logical-not.cc:2:7: note: add parentheses around left hand side expression to</div><div> silence</div><div> if (!5 < 4)</div><div> ^</div>
<div> ( )</div></div><div class="HOEnZb"><div class="h5"><div><br></div><div>On Mon, Oct 15, 2012 at 12:37 PM, Sean Silva <span dir="ltr"><<a href="mailto:silvas@purdue.edu" target="_blank" class="cremed">silvas@purdue.edu</a>></span> wrote:</div>
<div><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>> First, we have a fixit for (!expr1 < expr2). I'm recommending suggesting that maybe the user intended to write !(expr1 < expr2) instead, which is always at least as correct as the other proposed fixit, (expr1 >= expr2), and sometimes significantly better (if either expr is floating-point).<br>
<br>
</div>I agree, the intention of the programmer was clearly !(x<5).<br>
<div><br>
> More to the point, you're conflating two separate issues.<br>
<br>
</div>I must have miscommunicated, as my point was unrelated to either of<br>
those things. Really what I was trying to get across is that even the<br>
suggestions of !(x<5) could lead to a "subtle bug", Re your comment:<br>
<div>"We don't want to find bugs in users' code only to replace them with<br>
</div>even more subtle bugs". Even if the conversion is not UB (thanks for<br>
correcting me there), it is still likely to introduce a subtle bug.<br>
<br>
I think that the criterion for the suggestion should be "what the<br>
programmer intended", so that !(x<5) is clearly the preferable<br>
suggestion. IMO suggesting x>=5 goes a bit beyond the realm of<br>
"programmer intent" and into "trying to simplify your code with a<br>
suggestion as to how you should phrase a comparison".<br>
<span><font color="#888888"><br>
-- Sean Silva<br>
</font></span><div><div><br>
On Mon, Oct 15, 2012 at 2:07 PM, Stephen Canon <<a href="mailto:scanon@apple.com" target="_blank" class="cremed">scanon@apple.com</a>> wrote:<br>
><br>
> On Oct 15, 2012, at 1:37 PM, Sean Silva <<a href="mailto:silvas@purdue.edu" target="_blank" class="cremed">silvas@purdue.edu</a>> wrote:<br>
><br>
>>> The compiler shouldn't be in the business of suggesting semantically incorrect transformations to the user, especially when there's a perfectly good alternative available.<br>
>>><br>
>>> We don't want to find bugs in users' code only to replace them with even more subtle bugs. For example, conver<br>
>><br>
>> While I agree with you, there is a certain balance to be had. Even<br>
>> converting it to !(x < 5) could cause issues if the conversion between<br>
>> integer and float causes undefined behavior by not being<br>
>> representable. I forget which direction the conversion for (f < 5)<br>
>> would go (int->float or float->int), but either operand could be<br>
>> outside the domain of the other (replacing 5 by INT_MAX, for example,<br>
>> or having f be 1e30).<br>
><br>
> int -> float, and conversions from int to float are always defined (assuming IEEE-754 floating point). The conversion may be inexact, and may even overflow on platforms with 128-bit integers, but it is always defined.<br>
><br>
> More to the point, you're conflating two separate issues.<br>
><br>
> First, we have a fixit for (!expr1 < expr2). I'm recommending suggesting that maybe the user intended to write !(expr1 < expr2) instead, which is always at least as correct as the other proposed fixit, (expr1 >= expr2), and sometimes significantly better (if either expr is floating-point).<br>
><br>
> Possibly warning the user of inexact conversions of integer constants to floating-point is an entirely separate issue (worth investigating; it would be neat if the compiler warned on (float)x < INT_MAX by pointing out that the comparison is actually being performed against 0x80000000.0 instead of 0x7ffffff.0, which isn't representable in single-precision, but that's an entirely separate can of worms).<br>
><br>
> - Steve<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div>
</div></div><br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div>