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

[Richard Trieu]

On Tue, Oct 16, 2012 at 9:35 PM, Chandler Carruth <chandlerc at google.com>wrote:

> On Tue, Oct 16, 2012 at 3:34 PM, Richard Trieu <rtrieu at google.com> wrote:
>
>> Changed the fix-it to suggest parentheses around the comparison instead
>> of switching the comparison operator.
>
>
> 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.
>
> 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.
>

Are you suggesting this warning be extended to class types?  Currently, the
warning only checks for the built-in logical not operator.   Overloaded
operators are treated as function calls and won't trigger this warning.

>
> 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.
>
> -Chandler
>
>
>> logical-not.cc:2:7: warning: logical not is only applied to the left hand
>> side
>>        of this comparison [-Wlogical-not-compare]
>>   if (!5 < 4)
>>       ^  ~
>> logical-not.cc:2:7: note: add parentheses after the '!' to evaluate the
>>       condition first
>>   if (!5 < 4)
>>       ^
>>        (    )
>> logical-not.cc:2:7: note: add parentheses around left hand side
>> expression to
>>       silence
>>   if (!5 < 4)
>>       ^
>>       ( )
>>
>> On Mon, Oct 15, 2012 at 12:37 PM, Sean Silva <silvas at purdue.edu> wrote:
>>
>>> > 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
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121017/05261fdd/attachment.html>