Do you have numbers on the true positive rate of this new warning (give that it's on by default)?
On Mon, Jun 10, 2013 at 11:52 AM, Richard Trieu <[email protected]> wrote: > Author: rtrieu > Date: Mon Jun 10 13:52:07 2013 > New Revision: 183688 > > URL: http://llvm.org/viewvc/llvm-project?rev=183688&view=rev > Log: > 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. > > Added: > cfe/trunk/test/SemaCXX/warn-logical-not-compare.cpp > Modified: > cfe/trunk/include/clang/Basic/DiagnosticGroups.td > cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td > cfe/trunk/lib/Sema/SemaExpr.cpp > > Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=183688&r1=183687&r2=183688&view=diff > > ============================================================================== > --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original) > +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Mon Jun 10 13:52:07 > 2013 > @@ -136,6 +136,7 @@ def FourByteMultiChar : DiagGroup<"four- > 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<"duplic > def ParenthesesOnEquality : DiagGroup<"parentheses-equality">; > def Parentheses : DiagGroup<"parentheses", > [LogicalOpParentheses, > + LogicalNotParentheses, > BitwiseOpParentheses, > ShiftOpParentheses, > OverloadedShiftOpParentheses, > > Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=183688&r1=183687&r2=183688&view=diff > > ============================================================================== > --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) > +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Jun 10 > 13:52:07 2013 > @@ -4435,6 +4435,14 @@ def warn_null_in_comparison_operation : > "%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< > > Modified: cfe/trunk/lib/Sema/SemaExpr.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=183688&r1=183687&r2=183688&view=diff > > ============================================================================== > --- cfe/trunk/lib/Sema/SemaExpr.cpp (original) > +++ cfe/trunk/lib/Sema/SemaExpr.cpp Mon Jun 10 13:52:07 2013 > @@ -7250,6 +7250,45 @@ static void diagnoseObjCLiteralCompariso > } > } > > +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(Expr > Expr *RHSStripped = RHS.get()->IgnoreParenImpCasts(); > > checkEnumComparison(*this, Loc, LHS.get(), RHS.get()); > + diagnoseLogicalNotOnLHSofComparison(*this, LHS, RHS, Loc, OpaqueOpc); > > if (!LHSType->hasFloatingRepresentation() && > !(LHSType->isBlockPointerType() && IsRelational) && > > Added: cfe/trunk/test/SemaCXX/warn-logical-not-compare.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-logical-not-compare.cpp?rev=183688&view=auto > > ============================================================================== > --- cfe/trunk/test/SemaCXX/warn-logical-not-compare.cpp (added) > +++ cfe/trunk/test/SemaCXX/warn-logical-not-compare.cpp Mon Jun 10 > 13:52:07 2013 > @@ -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; > +} > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
