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@-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}}
+
+ 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}}
+
+ 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}}
+
+ 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}}
+
+ 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}}
+
+ 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}}
+
+ 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}}
+
+ 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) &&
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@-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}}
+
+ 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}}
+
+ 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}}
+
+ 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}}
+
+ 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}}
+
+ 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}}
+
+ 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}}
+
+ 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) &&
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
