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

[Chandler Carruth]

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.

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/20121016/04464d56/attachment.html>