r183688 - Add a new warning, -Wlogical-not-parentheses, to -Wparentheses.
Richard Trieu
rtrieu at google.com
Mon Jun 10 16:23:40 PDT 2013
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 <thakis at chromium.org> 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 <rtrieu at google.com> 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 at -1 {{logical not is only applied to the left hand
>> side of this comparison}}
>> + // expected-note at -2 {{add parentheses after the '!' to evaluate the
>> comparison first}}
>> + // expected-note at -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 at -1 {{logical not is only applied to the left hand
>> side of this comparison}}
>> + // expected-note at -2 {{add parentheses after the '!' to evaluate the
>> comparison first}}
>> + // expected-note at -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 at -1 {{logical not is only applied to the left hand
>> side of this comparison}}
>> + // expected-note at -2 {{add parentheses after the '!' to evaluate the
>> comparison first}}
>> + // expected-note at -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 at -1 {{logical not is only applied to the left hand
>> side of this comparison}}
>> + // expected-note at -2 {{add parentheses after the '!' to evaluate the
>> comparison first}}
>> + // expected-note at -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 at -1 {{logical not is only applied to the left hand
>> side of this comparison}}
>> + // expected-note at -2 {{add parentheses after the '!' to evaluate the
>> comparison first}}
>> + // expected-note at -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 at -1 {{logical not is only applied to the left hand
>> side of this comparison}}
>> + // expected-note at -2 {{add parentheses after the '!' to evaluate the
>> comparison first}}
>> + // expected-note at -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 at -1 {{logical not is only applied to the left hand
>> side of this comparison}}
>> + // expected-note at -2 {{add parentheses after the '!' to evaluate the
>> comparison first}}
>> + // expected-note at -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
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130610/72e420cc/attachment.html>
More information about the cfe-commits
mailing list