[cfe-commits] [Patch] Add new warning for logical not on LHS of comparison

[Sean Silva]

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

I agree, the intention of the programmer was clearly !(x<5).

> More to the point, you're conflating two separate issues.

I must have miscommunicated, as my point was unrelated to either of
those things. Really what I was trying to get across is that even the
suggestions of !(x<5) could lead to a "subtle bug", Re your comment:
"We don't want to find bugs in users' code only to replace them with
even more subtle bugs". Even if the conversion is not UB (thanks for
correcting me there), it is still likely to introduce a subtle bug.

I think that the criterion for the suggestion should be "what the
programmer intended", so that !(x<5) is clearly the preferable
suggestion. IMO suggesting x>=5 goes a bit beyond the realm of
"programmer intent" and into "trying to simplify your code with a
suggestion as to how you should phrase a comparison".

-- Sean Silva

On Mon, Oct 15, 2012 at 2:07 PM, Stephen Canon <scanon at apple.com> wrote:
>
> On Oct 15, 2012, at 1:37 PM, Sean Silva <silvas at purdue.edu> wrote:
>
>>> 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.
>>>
>>> We don't want to find bugs in users' code only to replace them with even more subtle bugs. For example, conver
>>
>> While I agree with you, there is a certain balance to be had. Even
>> converting it to !(x < 5) could cause issues if the conversion between
>> integer and float causes undefined behavior by not being
>> representable. I forget which direction the conversion for (f < 5)
>> would go (int->float or float->int), but either operand could be
>> outside the domain of the other (replacing 5 by INT_MAX, for example,
>> or having f be 1e30).
>
> 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.
>
> More to the point, you're conflating two separate issues.
>
> 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).
>
> 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).
>
> - Steve