Thanks!
On Mon, Jun 10, 2013 at 4:23 PM, Richard Trieu <[email protected]> wrote: > Close to 100% true positive. That is hundreds of bugs found in millions > of lines of code. For most cases, evaluating the comparison first was the > correct change. For some others, the '!' was a typo and removed. A few > cases involved macros. > > I am including cases where the code just happens to be correct. For > instance, !x > 0 is equivalent to !(x > 0). Clang still warns so people > won't rely on luck for code correctness. > > > On Mon, Jun 10, 2013 at 1:22 PM, Nico Weber <[email protected]> wrote: > >> 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
