[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]

  Change "... to silence" to "... to silence this warning"

Hi rsmith,

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

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D505?vs=1210&id=1464#toc

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

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) &&
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 this warning}}
+
+  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 this warning}}
+
+  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 this warning}}
+
+  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 this warning}}
+
+  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 this warning}}
+
+  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 this warning}}
+
+  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 this warning}}
+
+  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 this warning">;
+
 def err_invalid_this_use : Error<
   "invalid use of 'this' outside of a non-static member function">;
 def err_this_static_member_func : Error<
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D505.2.patch
Type: text/x-patch
Size: 6336 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130401/343fc534/attachment.bin>