[PATCH] New warning to detect when a logical not is applied to the LHS of a comparison when the intent was for the logical not to apply to the whole comparison

[Richard Trieu]

Previous discussion about this warning here:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20121008/066010.html

This warning detects code such as "if (!x < y)" when it was intended to be "if (!(x < y))".  It comes with two notes and fixits, one to add parens around the comparison, and the other to add parens around the LHS to silence the warning.

Summary of discussions:
1) This could be grouped under -Wparentheses or other existing diagnostic groups.
2) Possible removal of the second note which suggests how to silence this warning.
3) Warning has already been run on millions of lines of code, firing hundreds of times.  No false positives, but several cases of just happened to work.
4) Only catches on built-in operators.  Doesn't do analysis on overloaded operators.
5) No longer suggest opposite operator to so that subtle changes to FP behavior doesn't creep in.

http://llvm-reviews.chandlerc.com/D505

Files:
  test/SemaCXX/warn-logical-not-compare.cpp
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp

Index: test/SemaCXX/warn-logical-not-compare.cpp
===================================================================
--- test/SemaCXX/warn-logical-not-compare.cpp
+++ test/SemaCXX/warn-logical-not-compare.cpp
@@ -0,0 +1,69 @@
+// RUN: %clang_cc1 -fsyntax-only -Wlogical-not-compare -verify %s
+
+bool getBool();
+int getInt();
+
+bool test1(int i1, int i2, bool b1, bool b2) {
+  bool ret;
+
+  ret = !i1 == i2;
+  // expected-warning at -1 {{logical not is only applied to the left hand side of this comparison}}
+  // expected-note at -2 {{add parentheses after the '!' to evaluate the comparison first}}
+  // expected-note at -3 {{add parentheses around left hand side expression to silence}}
+
+  ret = !i1 != i2;
+  //expected-warning at -1 {{logical not is only applied to the left hand side of this comparison}}
+  // expected-note at -2 {{add parentheses after the '!' to evaluate the comparison first}}
+  // expected-note at -3 {{add parentheses around left hand side expression to silence}}
+
+  ret = !i1 < i2;
+  //expected-warning at -1 {{logical not is only applied to the left hand side of this comparison}}
+  // expected-note at -2 {{add parentheses after the '!' to evaluate the comparison first}}
+  // expected-note at -3 {{add parentheses around left hand side expression to silence}}
+
+  ret = !i1 > i2;
+  //expected-warning at -1 {{logical not is only applied to the left hand side of this comparison}}
+  // expected-note at -2 {{add parentheses after the '!' to evaluate the comparison first}}
+  // expected-note at -3 {{add parentheses around left hand side expression to silence}}
+
+  ret = !i1 <= i2;
+  //expected-warning at -1 {{logical not is only applied to the left hand side of this comparison}}
+  // expected-note at -2 {{add parentheses after the '!' to evaluate the comparison first}}
+  // expected-note at -3 {{add parentheses around left hand side expression to silence}}
+
+  ret = !i1 >= i2;
+  //expected-warning at -1 {{logical not is only applied to the left hand side of this comparison}}
+  // expected-note at -2 {{add parentheses after the '!' to evaluate the comparison first}}
+  // expected-note at -3 {{add parentheses around left hand side expression to silence}}
+
+  ret = i1 == i2;
+  ret = i1 != i2;
+  ret = i1 < i2;
+  ret = i1 > i2;
+  ret = i1 <= i2;
+  ret = i1 >= i2;
+
+  // Warning silenced by parens.
+  ret = (!i1) == i2;
+  ret = (!i1) != i2;
+  ret = (!i1) < i2;
+  ret = (!i1) > i2;
+  ret = (!i1) <= i2;
+  ret = (!i1) >= i2;
+
+  ret = !b1 == b2;
+  ret = !b1 != b2;
+  ret = !b1 < b2;
+  ret = !b1 > b2;
+  ret = !b1 <= b2;
+  ret = !b1 >= b2;
+
+  ret = !getInt() == i1;
+  // expected-warning at -1 {{logical not is only applied to the left hand side of this comparison}}
+  // expected-note at -2 {{add parentheses after the '!' to evaluate the comparison first}}
+  // expected-note at -3 {{add parentheses around left hand side expression to silence}}
+
+  ret = (!getInt()) == i1;
+  ret = !getBool() == b1;
+  return ret;
+}
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -4261,6 +4261,14 @@
   "%select{(%1 and NULL)|(NULL and %1)}0">,
   InGroup<NullArithmetic>;
 
+def warn_logical_not_on_lhs_of_comparison : Warning<
+  "logical not is only applied to the left hand side of this comparison">,
+  InGroup<DiagGroup<"logical-not-compare">>;
+def note_logical_not_fix : Note<
+  "add parentheses after the '!' to evaluate the comparison first">;
+def note_logical_not_silence_with_parens : Note<
+  "add parentheses around left hand side expression to silence">;
+
 def err_invalid_this_use : Error<
   "invalid use of 'this' outside of a non-static member function">;
 def err_this_static_member_func : Error<
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -7014,6 +7014,45 @@
   }
 }
 
+static void diagnoseLogicalNotOnLHSofComparison(Sema &S, ExprResult &LHS,
+                                                ExprResult &RHS,
+                                                SourceLocation Loc,
+                                                unsigned OpaqueOpc) {
+  // This checking requires bools.
+  if (!S.getLangOpts().Bool) return;
+
+  // Only check if the right hand side is non-bool arithmetic type.
+  if (RHS.get()->getType()->isBooleanType()) return;
+
+  // Check that left hand side is !something.
+  UnaryOperator *UO = dyn_cast<UnaryOperator>(LHS.get());
+  if (!UO || UO->getOpcode() != UO_LNot) return;
+
+  // Make sure that the something in !something is not bool.
+  Expr *SubExpr = UO->getSubExpr()->IgnoreImpCasts();
+  if (SubExpr->getType()->isBooleanType()) return;
+
+  // Emit warning.
+  S.Diag(UO->getOperatorLoc(), diag::warn_logical_not_on_lhs_of_comparison)
+      << Loc;
+
+  // First note suggest !(x < y)
+  SourceLocation FirstOpen = SubExpr->getLocStart();
+  SourceLocation FirstClose = RHS.get()->getSourceRange().getEnd();
+  FirstClose = S.getPreprocessor().getLocForEndOfToken(FirstClose);
+  S.Diag(UO->getOperatorLoc(), diag::note_logical_not_fix)
+      << FixItHint::CreateInsertion(FirstOpen, "(")
+      << FixItHint::CreateInsertion(FirstClose, ")");
+
+  // Second note suggests (!x) < y
+  SourceLocation SecondOpen = LHS.get()->getLocStart();
+  SourceLocation SecondClose = LHS.get()->getSourceRange().getEnd();
+  SecondClose = S.getPreprocessor().getLocForEndOfToken(SecondClose);
+  S.Diag(UO->getOperatorLoc(), diag::note_logical_not_silence_with_parens)
+      << FixItHint::CreateInsertion(SecondOpen, "(")
+      << FixItHint::CreateInsertion(SecondClose, ")");
+}
+
 // C99 6.5.8, C++ [expr.rel]
 QualType Sema::CheckCompareOperands(ExprResult &LHS, ExprResult &RHS,
                                     SourceLocation Loc, unsigned OpaqueOpc,
@@ -7034,6 +7073,7 @@
   Expr *RHSStripped = RHS.get()->IgnoreParenImpCasts();
 
   checkEnumComparison(*this, Loc, LHS.get(), RHS.get());
+  diagnoseLogicalNotOnLHSofComparison(*this, LHS, RHS, Loc, OpaqueOpc);
 
   if (!LHSType->hasFloatingRepresentation() &&
       !(LHSType->isBlockPointerType() && IsRelational) &&
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D505.1.patch
Type: text/x-patch
Size: 6232 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130307/2a12847e/attachment.bin>