r183688 - Add a new warning, -Wlogical-not-parentheses, to -Wparentheses.
Nico Weber
thakis at chromium.org
Mon Jun 10 17:13:11 PDT 2013
Thanks!
On Mon, Jun 10, 2013 at 4:23 PM, Richard Trieu <rtrieu at google.com> 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 <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/1c341c2d/attachment.html>
More information about the cfe-commits
mailing list