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

[Richard Trieu]

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121016/29aac926/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: logical-not-compare2.patch
Type: application/octet-stream
Size: 6257 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121016/29aac926/attachment.obj>