r183688 - Add a new warning, -Wlogical-not-parentheses, to -Wparentheses.
Nico Weber
thakis at chromium.org
Mon Jun 10 13:22:18 PDT 2013
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/5bc2fc1b/attachment.html>
More information about the cfe-commits
mailing list