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

[Chandler Carruth]

On Wed, Oct 17, 2012 at 2:08 PM, Richard Trieu <rtrieu at google.com> wrote:

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

No, sorry. I'm neither suggesting it, or suggesting not doing it, I simply
didn't check which the patch did and wanted to cover all my bases. =] Feel
free to ignore any cases in my comment which simply don't apply. =]



>
>> 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/3f72864f/attachment.html>