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

[Sean Silva]

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

This warning text seems weird to me. The core of the matter in my eyes
is "numerical comparison against result of logical not", not really
that the logical not is only applied to the left hand side per se.
I.e. the reason we are flagging this is that the comparison looks
suspicious.

> logical-not.cc:2:7: note: add parentheses after the '!' to evaluate the
>       condition first
>   if (!5 < 4)
>       ^
>        (    )

What do you think about the wording "add parentheses after the '!' to
negate the result of the comparison"? Also, it just seems outright
wrong to say "the condition" when meaning only the comparison, and not
the whole condition of the if.

+  ret = !getInt() == i1; // \
+// expected-warning{{logical not is only applied to the left hand
side of this comparison}} \
+// expected-note{{add parentheses after the '!' to evaluate the
condition first}} \
+// expected-note{{add parentheses around left hand side expression to silence}}

You might be able to make this a bit nicer if you use -verify's
relative line specification functionality `// expected-warning at +1
{{...}}` to expect the warning on the next line (or @+N in general for
the N'th line).

-- Sean Silva

On Tue, Oct 16, 2012 at 6: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.
>
> 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
>
>