Changed the fix-it to suggest parentheses around the comparison instead of switching the comparison operator.<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><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">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 class="im">> 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 class="im"><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 class="im">"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 class="HOEnZb"><font color="#888888"><br>
-- Sean Silva<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Mon, Oct 15, 2012 at 2:07 PM, Stephen Canon <<a href="mailto:scanon@apple.com">scanon@apple.com</a>> wrote:<br>
><br>
> On Oct 15, 2012, at 1:37 PM, Sean Silva <<a href="mailto:silvas@purdue.edu">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">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>
</div></div></blockquote></div><br></div>