Add a new warning, -Wlogical-not-parentheses, to -Wparentheses.

This warning triggers on the logical not of a non-boolean expression on the
left hand side of comparison.  Often, the user meant to negate the comparison,
not just the left hand side of the comparison.  Two notes are also emitted,
the first with a fix-it to add parentheses around the comparison, and the other
to put parenthesis around the not expression to silence the warning.

bool not_equal(int x, int y) {
  return !x == y;  // warn here
}

  return !(x == y);  // first fix-it, to negate comparison.

  return (!x) == y;  // second fix-it, to silence warning.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@183688 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td
index 8259e2e368086a798e7f5f82e62aca75f7fb31f9..99b342454c42b7cabc8ab4817386af8c2c1d9df0 100644 (file)
--- a/include/clang/Basic/DiagnosticGroups.td
+++ b/include/clang/Basic/DiagnosticGroups.td
@@ -136,6 +136,7 @@ def FourByteMultiChar : DiagGroup<"four-char-constants">;
 def GlobalConstructors : DiagGroup<"global-constructors">;
 def BitwiseOpParentheses: DiagGroup<"bitwise-op-parentheses">;
 def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">;
+def LogicalNotParentheses: DiagGroup<"logical-not-parentheses">;
 def ShiftOpParentheses: DiagGroup<"shift-op-parentheses">;
 def OverloadedShiftOpParentheses: DiagGroup<"overloaded-shift-op-parentheses">;
 def DanglingElse: DiagGroup<"dangling-else">;
@@ -368,6 +369,7 @@ def DuplicateArgDecl : DiagGroup<"duplicate-method-arg">;
 def ParenthesesOnEquality : DiagGroup<"parentheses-equality">;
 def Parentheses : DiagGroup<"parentheses",
                             [LogicalOpParentheses,
+                             LogicalNotParentheses,
                              BitwiseOpParentheses,
                              ShiftOpParentheses,
                              OverloadedShiftOpParentheses,

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index f79233e52e16218facea20e5cd00d04845970a8f..b9c0518decee88231fe4761b70edc4e24455af37 100644 (file)
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4435,6 +4435,14 @@ def warn_null_in_comparison_operation : Warning<
   "%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<LogicalNotParentheses>;
+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<

diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index 610dc28e3d9cedaab104358790f16a1375bece9b..f3fc11395448b7491e457845664bee24d49b4810 100644 (file)
--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -7250,6 +7250,45 @@ static void diagnoseObjCLiteralComparison(Sema &S, SourceLocation Loc,
   }
 }
 
+static void diagnoseLogicalNotOnLHSofComparison(Sema &S, ExprResult &LHS,
+                                                ExprResult &RHS,
+                                                SourceLocation Loc,
+                                                unsigned OpaqueOpc) {
+  // This checking requires bools.
+  if (!S.getLangOpts().Bool) return;
+
+  // Check that left hand side is !something.
+  UnaryOperator *UO = dyn_cast<UnaryOperator>(LHS.get());
+  if (!UO || UO->getOpcode() != UO_LNot) return;
+
+  // Only check if the right hand side is non-bool arithmetic type.
+  if (RHS.get()->getType()->isBooleanType()) 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()->getLocEnd();
+  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()->getLocEnd();
+  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,
@@ -7270,6 +7309,7 @@ QualType Sema::CheckCompareOperands(ExprResult &LHS, ExprResult &RHS,
   Expr *RHSStripped = RHS.get()->IgnoreParenImpCasts();
 
   checkEnumComparison(*this, Loc, LHS.get(), RHS.get());
+  diagnoseLogicalNotOnLHSofComparison(*this, LHS, RHS, Loc, OpaqueOpc);
 
   if (!LHSType->hasFloatingRepresentation() &&
       !(LHSType->isBlockPointerType() && IsRelational) &&

diff --git a/test/SemaCXX/warn-logical-not-compare.cpp b/test/SemaCXX/warn-logical-not-compare.cpp
new file mode 100644 (file)
index 0000000..365a028
--- /dev/null
+++ b/test/SemaCXX/warn-logical-not-compare.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -fsyntax-only -Wlogical-not-parentheses -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wlogical-not-parentheses -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+bool getBool();
+int getInt();
+
+bool test1(int i1, int i2, bool b1, bool b2) {
+  bool ret;
+
+  ret = !i1 == i2;
+  // expected-warning@-1 {{logical not is only applied to the left hand side of this comparison}}
+  // expected-note@-2 {{add parentheses after the '!' to evaluate the comparison first}}
+  // expected-note@-3 {{add parentheses around left hand side expression to silence this warning}}
+  // CHECK: to evaluate the comparison first
+  // CHECK: fix-it:"{{.*}}":{10:10-10:10}:"("
+  // CHECK: fix-it:"{{.*}}":{10:18-10:18}:")"
+  // CHECK: to silence this warning
+  // CHECK: fix-it:"{{.*}}":{10:9-10:9}:"("
+  // CHECK: fix-it:"{{.*}}":{10:12-10:12}:")"
+
+  ret = !i1 != i2;
+  //expected-warning@-1 {{logical not is only applied to the left hand side of this comparison}}
+  // expected-note@-2 {{add parentheses after the '!' to evaluate the comparison first}}
+  // expected-note@-3 {{add parentheses around left hand side expression to silence this warning}}
+  // CHECK: to evaluate the comparison first
+  // CHECK: fix-it:"{{.*}}":{21:10-21:10}:"("
+  // CHECK: fix-it:"{{.*}}":{21:18-21:18}:")"
+  // CHECK: to silence this warning
+  // CHECK: fix-it:"{{.*}}":{21:9-21:9}:"("
+  // CHECK: fix-it:"{{.*}}":{21:12-21:12}:")"
+
+  ret = !i1 < i2;
+  //expected-warning@-1 {{logical not is only applied to the left hand side of this comparison}}
+  // expected-note@-2 {{add parentheses after the '!' to evaluate the comparison first}}
+  // expected-note@-3 {{add parentheses around left hand side expression to silence this warning}}
+  // CHECK: to evaluate the comparison first
+  // CHECK: fix-it:"{{.*}}":{32:10-32:10}:"("
+  // CHECK: fix-it:"{{.*}}":{32:17-32:17}:")"
+  // CHECK: to silence this warning
+  // CHECK: fix-it:"{{.*}}":{32:9-32:9}:"("
+  // CHECK: fix-it:"{{.*}}":{32:12-32:12}:")"
+
+  ret = !i1 > i2;
+  //expected-warning@-1 {{logical not is only applied to the left hand side of this comparison}}
+  // expected-note@-2 {{add parentheses after the '!' to evaluate the comparison first}}
+  // expected-note@-3 {{add parentheses around left hand side expression to silence this warning}}
+  // CHECK: to evaluate the comparison first
+  // CHECK: fix-it:"{{.*}}":{43:10-43:10}:"("
+  // CHECK: fix-it:"{{.*}}":{43:17-43:17}:")"
+  // CHECK: to silence this warning
+  // CHECK: fix-it:"{{.*}}":{43:9-43:9}:"("
+  // CHECK: fix-it:"{{.*}}":{43:12-43:12}:")"
+
+  ret = !i1 <= i2;
+  //expected-warning@-1 {{logical not is only applied to the left hand side of this comparison}}
+  // expected-note@-2 {{add parentheses after the '!' to evaluate the comparison first}}
+  // expected-note@-3 {{add parentheses around left hand side expression to silence this warning}}
+  // CHECK: to evaluate the comparison first
+  // CHECK: fix-it:"{{.*}}":{54:10-54:10}:"("
+  // CHECK: fix-it:"{{.*}}":{54:18-54:18}:")"
+  // CHECK: to silence this warning
+  // CHECK: fix-it:"{{.*}}":{54:9-54:9}:"("
+  // CHECK: fix-it:"{{.*}}":{54:12-54:12}:")"
+
+  ret = !i1 >= i2;
+  //expected-warning@-1 {{logical not is only applied to the left hand side of this comparison}}
+  // expected-note@-2 {{add parentheses after the '!' to evaluate the comparison first}}
+  // expected-note@-3 {{add parentheses around left hand side expression to silence this warning}}
+  // CHECK: to evaluate the comparison first
+  // CHECK: fix-it:"{{.*}}":{65:10-65:10}:"("
+  // CHECK: fix-it:"{{.*}}":{65:18-65:18}:")"
+  // CHECK: to silence this warning
+  // CHECK: fix-it:"{{.*}}":{65:9-65:9}:"("
+  // CHECK: fix-it:"{{.*}}":{65:12-65:12}:")"
+
+  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@-1 {{logical not is only applied to the left hand side of this comparison}}
+  // expected-note@-2 {{add parentheses after the '!' to evaluate the comparison first}}
+  // expected-note@-3 {{add parentheses around left hand side expression to silence this warning}}
+  // CHECK: to evaluate the comparison first
+  // CHECK: fix-it:"{{.*}}":{98:10-98:10}:"("
+  // CHECK: fix-it:"{{.*}}":{98:24-98:24}:")"
+  // CHECK: to silence this warning
+  // CHECK: fix-it:"{{.*}}":{98:9-98:9}:"("
+  // CHECK: fix-it:"{{.*}}":{98:18-98:18}:")"
+
+  ret = (!getInt()) == i1;
+  ret = !getBool() == b1;
+  return ret;
+}