<div dir="ltr">Do you have numbers on the true positive rate of this new warning (give that it's on by default)?</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jun 10, 2013 at 11:52 AM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: rtrieu<br>
Date: Mon Jun 10 13:52:07 2013<br>
New Revision: 183688<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=183688&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=183688&view=rev</a><br>
Log:<br>
Add a new warning, -Wlogical-not-parentheses, to -Wparentheses.<br>
<br>
This warning triggers on the logical not of a non-boolean expression on the<br>
left hand side of comparison. Often, the user meant to negate the comparison,<br>
not just the left hand side of the comparison. Two notes are also emitted,<br>
the first with a fix-it to add parentheses around the comparison, and the other<br>
to put parenthesis around the not expression to silence the warning.<br>
<br>
bool not_equal(int x, int y) {<br>
return !x == y; // warn here<br>
}<br>
<br>
return !(x == y); // first fix-it, to negate comparison.<br>
<br>
return (!x) == y; // second fix-it, to silence warning.<br>
<br>
Added:<br>
cfe/trunk/test/SemaCXX/warn-logical-not-compare.cpp<br>
Modified:<br>
cfe/trunk/include/clang/Basic/DiagnosticGroups.td<br>
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
cfe/trunk/lib/Sema/SemaExpr.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=183688&r1=183687&r2=183688&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=183688&r1=183687&r2=183688&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Mon Jun 10 13:52:07 2013<br>
@@ -136,6 +136,7 @@ def FourByteMultiChar : DiagGroup<"four-<br>
def GlobalConstructors : DiagGroup<"global-constructors">;<br>
def BitwiseOpParentheses: DiagGroup<"bitwise-op-parentheses">;<br>
def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">;<br>
+def LogicalNotParentheses: DiagGroup<"logical-not-parentheses">;<br>
def ShiftOpParentheses: DiagGroup<"shift-op-parentheses">;<br>
def OverloadedShiftOpParentheses: DiagGroup<"overloaded-shift-op-parentheses">;<br>
def DanglingElse: DiagGroup<"dangling-else">;<br>
@@ -368,6 +369,7 @@ def DuplicateArgDecl : DiagGroup<"duplic<br>
def ParenthesesOnEquality : DiagGroup<"parentheses-equality">;<br>
def Parentheses : DiagGroup<"parentheses",<br>
[LogicalOpParentheses,<br>
+ LogicalNotParentheses,<br>
BitwiseOpParentheses,<br>
ShiftOpParentheses,<br>
OverloadedShiftOpParentheses,<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=183688&r1=183687&r2=183688&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=183688&r1=183687&r2=183688&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Jun 10 13:52:07 2013<br>
@@ -4435,6 +4435,14 @@ def warn_null_in_comparison_operation :<br>
"%select{(%1 and NULL)|(NULL and %1)}0">,<br>
InGroup<NullArithmetic>;<br>
<br>
+def warn_logical_not_on_lhs_of_comparison : Warning<<br>
+ "logical not is only applied to the left hand side of this comparison">,<br>
+ InGroup<LogicalNotParentheses>;<br>
+def note_logical_not_fix : Note<<br>
+ "add parentheses after the '!' to evaluate the comparison first">;<br>
+def note_logical_not_silence_with_parens : Note<<br>
+ "add parentheses around left hand side expression to silence this warning">;<br>
+<br>
def err_invalid_this_use : Error<<br>
"invalid use of 'this' outside of a non-static member function">;<br>
def err_this_static_member_func : Error<<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaExpr.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=183688&r1=183687&r2=183688&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=183688&r1=183687&r2=183688&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Mon Jun 10 13:52:07 2013<br>
@@ -7250,6 +7250,45 @@ static void diagnoseObjCLiteralCompariso<br>
}<br>
}<br>
<br>
+static void diagnoseLogicalNotOnLHSofComparison(Sema &S, ExprResult &LHS,<br>
+ ExprResult &RHS,<br>
+ SourceLocation Loc,<br>
+ unsigned OpaqueOpc) {<br>
+ // This checking requires bools.<br>
+ if (!S.getLangOpts().Bool) return;<br>
+<br>
+ // Check that left hand side is !something.<br>
+ UnaryOperator *UO = dyn_cast<UnaryOperator>(LHS.get());<br>
+ if (!UO || UO->getOpcode() != UO_LNot) return;<br>
+<br>
+ // Only check if the right hand side is non-bool arithmetic type.<br>
+ if (RHS.get()->getType()->isBooleanType()) return;<br>
+<br>
+ // Make sure that the something in !something is not bool.<br>
+ Expr *SubExpr = UO->getSubExpr()->IgnoreImpCasts();<br>
+ if (SubExpr->getType()->isBooleanType()) return;<br>
+<br>
+ // Emit warning.<br>
+ S.Diag(UO->getOperatorLoc(), diag::warn_logical_not_on_lhs_of_comparison)<br>
+ << Loc;<br>
+<br>
+ // First note suggest !(x < y)<br>
+ SourceLocation FirstOpen = SubExpr->getLocStart();<br>
+ SourceLocation FirstClose = RHS.get()->getLocEnd();<br>
+ FirstClose = S.getPreprocessor().getLocForEndOfToken(FirstClose);<br>
+ S.Diag(UO->getOperatorLoc(), diag::note_logical_not_fix)<br>
+ << FixItHint::CreateInsertion(FirstOpen, "(")<br>
+ << FixItHint::CreateInsertion(FirstClose, ")");<br>
+<br>
+ // Second note suggests (!x) < y<br>
+ SourceLocation SecondOpen = LHS.get()->getLocStart();<br>
+ SourceLocation SecondClose = LHS.get()->getLocEnd();<br>
+ SecondClose = S.getPreprocessor().getLocForEndOfToken(SecondClose);<br>
+ S.Diag(UO->getOperatorLoc(), diag::note_logical_not_silence_with_parens)<br>
+ << FixItHint::CreateInsertion(SecondOpen, "(")<br>
+ << FixItHint::CreateInsertion(SecondClose, ")");<br>
+}<br>
+<br>
// C99 6.5.8, C++ [expr.rel]<br>
QualType Sema::CheckCompareOperands(ExprResult &LHS, ExprResult &RHS,<br>
SourceLocation Loc, unsigned OpaqueOpc,<br>
@@ -7270,6 +7309,7 @@ QualType Sema::CheckCompareOperands(Expr<br>
Expr *RHSStripped = RHS.get()->IgnoreParenImpCasts();<br>
<br>
checkEnumComparison(*this, Loc, LHS.get(), RHS.get());<br>
+ diagnoseLogicalNotOnLHSofComparison(*this, LHS, RHS, Loc, OpaqueOpc);<br>
<br>
if (!LHSType->hasFloatingRepresentation() &&<br>
!(LHSType->isBlockPointerType() && IsRelational) &&<br>
<br>
Added: cfe/trunk/test/SemaCXX/warn-logical-not-compare.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-logical-not-compare.cpp?rev=183688&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-logical-not-compare.cpp?rev=183688&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/test/SemaCXX/warn-logical-not-compare.cpp (added)<br>
+++ cfe/trunk/test/SemaCXX/warn-logical-not-compare.cpp Mon Jun 10 13:52:07 2013<br>
@@ -0,0 +1,112 @@<br>
+// RUN: %clang_cc1 -fsyntax-only -Wlogical-not-parentheses -verify %s<br>
+// RUN: %clang_cc1 -fsyntax-only -Wlogical-not-parentheses -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s<br>
+<br>
+bool getBool();<br>
+int getInt();<br>
+<br>
+bool test1(int i1, int i2, bool b1, bool b2) {<br>
+ bool ret;<br>
+<br>
+ ret = !i1 == i2;<br>
+ // expected-warning@-1 {{logical not is only applied to the left hand side of this comparison}}<br>
+ // expected-note@-2 {{add parentheses after the '!' to evaluate the comparison first}}<br>
+ // expected-note@-3 {{add parentheses around left hand side expression to silence this warning}}<br>
+ // CHECK: to evaluate the comparison first<br>
+ // CHECK: fix-it:"{{.*}}":{10:10-10:10}:"("<br>
+ // CHECK: fix-it:"{{.*}}":{10:18-10:18}:")"<br>
+ // CHECK: to silence this warning<br>
+ // CHECK: fix-it:"{{.*}}":{10:9-10:9}:"("<br>
+ // CHECK: fix-it:"{{.*}}":{10:12-10:12}:")"<br>
+<br>
+ ret = !i1 != i2;<br>
+ //expected-warning@-1 {{logical not is only applied to the left hand side of this comparison}}<br>
+ // expected-note@-2 {{add parentheses after the '!' to evaluate the comparison first}}<br>
+ // expected-note@-3 {{add parentheses around left hand side expression to silence this warning}}<br>
+ // CHECK: to evaluate the comparison first<br>
+ // CHECK: fix-it:"{{.*}}":{21:10-21:10}:"("<br>
+ // CHECK: fix-it:"{{.*}}":{21:18-21:18}:")"<br>
+ // CHECK: to silence this warning<br>
+ // CHECK: fix-it:"{{.*}}":{21:9-21:9}:"("<br>
+ // CHECK: fix-it:"{{.*}}":{21:12-21:12}:")"<br>
+<br>
+ ret = !i1 < i2;<br>
+ //expected-warning@-1 {{logical not is only applied to the left hand side of this comparison}}<br>
+ // expected-note@-2 {{add parentheses after the '!' to evaluate the comparison first}}<br>
+ // expected-note@-3 {{add parentheses around left hand side expression to silence this warning}}<br>
+ // CHECK: to evaluate the comparison first<br>
+ // CHECK: fix-it:"{{.*}}":{32:10-32:10}:"("<br>
+ // CHECK: fix-it:"{{.*}}":{32:17-32:17}:")"<br>
+ // CHECK: to silence this warning<br>
+ // CHECK: fix-it:"{{.*}}":{32:9-32:9}:"("<br>
+ // CHECK: fix-it:"{{.*}}":{32:12-32:12}:")"<br>
+<br>
+ ret = !i1 > i2;<br>
+ //expected-warning@-1 {{logical not is only applied to the left hand side of this comparison}}<br>
+ // expected-note@-2 {{add parentheses after the '!' to evaluate the comparison first}}<br>
+ // expected-note@-3 {{add parentheses around left hand side expression to silence this warning}}<br>
+ // CHECK: to evaluate the comparison first<br>
+ // CHECK: fix-it:"{{.*}}":{43:10-43:10}:"("<br>
+ // CHECK: fix-it:"{{.*}}":{43:17-43:17}:")"<br>
+ // CHECK: to silence this warning<br>
+ // CHECK: fix-it:"{{.*}}":{43:9-43:9}:"("<br>
+ // CHECK: fix-it:"{{.*}}":{43:12-43:12}:")"<br>
+<br>
+ ret = !i1 <= i2;<br>
+ //expected-warning@-1 {{logical not is only applied to the left hand side of this comparison}}<br>
+ // expected-note@-2 {{add parentheses after the '!' to evaluate the comparison first}}<br>
+ // expected-note@-3 {{add parentheses around left hand side expression to silence this warning}}<br>
+ // CHECK: to evaluate the comparison first<br>
+ // CHECK: fix-it:"{{.*}}":{54:10-54:10}:"("<br>
+ // CHECK: fix-it:"{{.*}}":{54:18-54:18}:")"<br>
+ // CHECK: to silence this warning<br>
+ // CHECK: fix-it:"{{.*}}":{54:9-54:9}:"("<br>
+ // CHECK: fix-it:"{{.*}}":{54:12-54:12}:")"<br>
+<br>
+ ret = !i1 >= i2;<br>
+ //expected-warning@-1 {{logical not is only applied to the left hand side of this comparison}}<br>
+ // expected-note@-2 {{add parentheses after the '!' to evaluate the comparison first}}<br>
+ // expected-note@-3 {{add parentheses around left hand side expression to silence this warning}}<br>
+ // CHECK: to evaluate the comparison first<br>
+ // CHECK: fix-it:"{{.*}}":{65:10-65:10}:"("<br>
+ // CHECK: fix-it:"{{.*}}":{65:18-65:18}:")"<br>
+ // CHECK: to silence this warning<br>
+ // CHECK: fix-it:"{{.*}}":{65:9-65:9}:"("<br>
+ // CHECK: fix-it:"{{.*}}":{65:12-65:12}:")"<br>
+<br>
+ ret = i1 == i2;<br>
+ ret = i1 != i2;<br>
+ ret = i1 < i2;<br>
+ ret = i1 > i2;<br>
+ ret = i1 <= i2;<br>
+ ret = i1 >= i2;<br>
+<br>
+ // Warning silenced by parens.<br>
+ ret = (!i1) == i2;<br>
+ ret = (!i1) != i2;<br>
+ ret = (!i1) < i2;<br>
+ ret = (!i1) > i2;<br>
+ ret = (!i1) <= i2;<br>
+ ret = (!i1) >= i2;<br>
+<br>
+ ret = !b1 == b2;<br>
+ ret = !b1 != b2;<br>
+ ret = !b1 < b2;<br>
+ ret = !b1 > b2;<br>
+ ret = !b1 <= b2;<br>
+ ret = !b1 >= b2;<br>
+<br>
+ ret = !getInt() == i1;<br>
+ // expected-warning@-1 {{logical not is only applied to the left hand side of this comparison}}<br>
+ // expected-note@-2 {{add parentheses after the '!' to evaluate the comparison first}}<br>
+ // expected-note@-3 {{add parentheses around left hand side expression to silence this warning}}<br>
+ // CHECK: to evaluate the comparison first<br>
+ // CHECK: fix-it:"{{.*}}":{98:10-98:10}:"("<br>
+ // CHECK: fix-it:"{{.*}}":{98:24-98:24}:")"<br>
+ // CHECK: to silence this warning<br>
+ // CHECK: fix-it:"{{.*}}":{98:9-98:9}:"("<br>
+ // CHECK: fix-it:"{{.*}}":{98:18-98:18}:")"<br>
+<br>
+ ret = (!getInt()) == i1;<br>
+ ret = !getBool() == b1;<br>
+ return ret;<br>
+}<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>