r285310 - Expand -Wlogical-not-parentheses to also fire on `!x & A`.
Nico Weber via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 27 09:32:07 PDT 2016
Author: nico
Date: Thu Oct 27 11:32:06 2016
New Revision: 285310
URL: http://llvm.org/viewvc/llvm-project?rev=285310&view=rev
Log:
Expand -Wlogical-not-parentheses to also fire on `!x & A`.
This is a misspelling of the intended !(x & A) negated bit test that happens in
practice every now and then.
I ran this on Chromium and all its dependencies, and it fired 0 times -- no
false or true positives, but it would've caught a bug in an in-progress change
that had to be caught by a Visual Studio warning instead.
https://reviews.llvm.org/D26035
Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Sema/SemaExpr.cpp
cfe/trunk/test/SemaCXX/warn-logical-not-compare.cpp
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=285310&r1=285309&r2=285310&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Oct 27 11:32:06 2016
@@ -5632,11 +5632,13 @@ def err_shift_rhs_only_vector : Error<
"requested shift is a vector of type %0 but the first operand is not a "
"vector (%1)">;
-def warn_logical_not_on_lhs_of_comparison : Warning<
- "logical not is only applied to the left hand side of this comparison">,
+def warn_logical_not_on_lhs_of_check : Warning<
+ "logical not is only applied to the left hand side of this "
+ "%select{comparison|bitwise operator}0">,
InGroup<LogicalNotParentheses>;
def note_logical_not_fix : Note<
- "add parentheses after the '!' to evaluate the comparison first">;
+ "add parentheses after the '!' to evaluate the "
+ "%select{comparison|bitwise operator}0 first">;
def note_logical_not_silence_with_parens : Note<
"add parentheses around left hand side expression to silence this warning">;
Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=285310&r1=285309&r2=285310&view=diff
==============================================================================
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Thu Oct 27 11:32:06 2016
@@ -8927,8 +8927,8 @@ public:
ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
BinaryOperatorKind Opc, bool isRelational);
QualType CheckBitwiseOperands( // C99 6.5.[10...12]
- ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
- bool IsCompAssign = false);
+ ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
+ BinaryOperatorKind Opc);
QualType CheckLogicalOperands( // C99 6.5.[13,14]
ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
BinaryOperatorKind Opc);
Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=285310&r1=285309&r2=285310&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Thu Oct 27 11:32:06 2016
@@ -9095,10 +9095,10 @@ static void diagnoseObjCLiteralCompariso
}
}
-static void diagnoseLogicalNotOnLHSofComparison(Sema &S, ExprResult &LHS,
- ExprResult &RHS,
- SourceLocation Loc,
- BinaryOperatorKind Opc) {
+/// Warns on !x < y, !x & y where !(x < y), !(x & y) was probably intended.
+static void diagnoseLogicalNotOnLHSofCheck(Sema &S, ExprResult &LHS,
+ ExprResult &RHS, SourceLocation Loc,
+ BinaryOperatorKind Opc) {
// Check that left hand side is !something.
UnaryOperator *UO = dyn_cast<UnaryOperator>(LHS.get()->IgnoreImpCasts());
if (!UO || UO->getOpcode() != UO_LNot) return;
@@ -9111,8 +9111,9 @@ static void diagnoseLogicalNotOnLHSofCom
if (SubExpr->isKnownToHaveBooleanValue()) return;
// Emit warning.
- S.Diag(UO->getOperatorLoc(), diag::warn_logical_not_on_lhs_of_comparison)
- << Loc;
+ bool IsBitwiseOp = Opc == BO_And || Opc == BO_Or || Opc == BO_Xor;
+ S.Diag(UO->getOperatorLoc(), diag::warn_logical_not_on_lhs_of_check)
+ << Loc << IsBitwiseOp;
// First note suggest !(x < y)
SourceLocation FirstOpen = SubExpr->getLocStart();
@@ -9121,6 +9122,7 @@ static void diagnoseLogicalNotOnLHSofCom
if (FirstClose.isInvalid())
FirstOpen = SourceLocation();
S.Diag(UO->getOperatorLoc(), diag::note_logical_not_fix)
+ << IsBitwiseOp
<< FixItHint::CreateInsertion(FirstOpen, "(")
<< FixItHint::CreateInsertion(FirstClose, ")");
@@ -9169,7 +9171,7 @@ QualType Sema::CheckCompareOperands(Expr
Expr *RHSStripped = RHS.get()->IgnoreParenImpCasts();
checkEnumComparison(*this, Loc, LHS.get(), RHS.get());
- diagnoseLogicalNotOnLHSofComparison(*this, LHS, RHS, Loc, Opc);
+ diagnoseLogicalNotOnLHSofCheck(*this, LHS, RHS, Loc, Opc);
if (!LHSType->hasFloatingRepresentation() &&
!(LHSType->isBlockPointerType() && IsRelational) &&
@@ -9675,10 +9677,14 @@ QualType Sema::CheckVectorLogicalOperand
return GetSignedVectorType(LHS.get()->getType());
}
-inline QualType Sema::CheckBitwiseOperands(
- ExprResult &LHS, ExprResult &RHS, SourceLocation Loc, bool IsCompAssign) {
+inline QualType Sema::CheckBitwiseOperands(ExprResult &LHS, ExprResult &RHS,
+ SourceLocation Loc,
+ BinaryOperatorKind Opc) {
checkArithmeticNull(*this, LHS, RHS, Loc, /*isCompare=*/false);
+ bool IsCompAssign =
+ Opc == BO_AndAssign || Opc == BO_OrAssign || Opc == BO_XorAssign;
+
if (LHS.get()->getType()->isVectorType() ||
RHS.get()->getType()->isVectorType()) {
if (LHS.get()->getType()->hasIntegerRepresentation() &&
@@ -9689,6 +9695,9 @@ inline QualType Sema::CheckBitwiseOperan
return InvalidOperands(Loc, LHS, RHS);
}
+ if (Opc == BO_And)
+ diagnoseLogicalNotOnLHSofCheck(*this, LHS, RHS, Loc, Opc);
+
ExprResult LHSResult = LHS, RHSResult = RHS;
QualType compType = UsualArithmeticConversions(LHSResult, RHSResult,
IsCompAssign);
@@ -11057,7 +11066,7 @@ ExprResult Sema::CreateBuiltinBinOp(Sour
checkObjCPointerIntrospection(*this, LHS, RHS, OpLoc);
case BO_Xor:
case BO_Or:
- ResultTy = CheckBitwiseOperands(LHS, RHS, OpLoc);
+ ResultTy = CheckBitwiseOperands(LHS, RHS, OpLoc, Opc);
break;
case BO_LAnd:
case BO_LOr:
@@ -11098,7 +11107,7 @@ ExprResult Sema::CreateBuiltinBinOp(Sour
case BO_OrAssign: // fallthrough
DiagnoseSelfAssignment(*this, LHS.get(), RHS.get(), OpLoc);
case BO_XorAssign:
- CompResultTy = CheckBitwiseOperands(LHS, RHS, OpLoc, true);
+ CompResultTy = CheckBitwiseOperands(LHS, RHS, OpLoc, Opc);
CompLHSTy = CompResultTy;
if (!CompResultTy.isNull() && !LHS.isInvalid() && !RHS.isInvalid())
ResultTy = CheckAssignmentOperands(LHS.get(), RHS, OpLoc, CompResultTy);
Modified: 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=285310&r1=285309&r2=285310&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/warn-logical-not-compare.cpp (original)
+++ cfe/trunk/test/SemaCXX/warn-logical-not-compare.cpp Thu Oct 27 11:32:06 2016
@@ -1,5 +1,5 @@
// 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
+// RUN: not %clang_cc1 -fsyntax-only -Wlogical-not-parentheses -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
bool getBool();
int getInt();
@@ -188,6 +188,45 @@ bool test2 (E e) {
return ret;
}
+
+bool test_bitwise_op(int x) {
+ bool ret;
+
+ ret = !x & 1;
+ // expected-warning at -1 {{logical not is only applied to the left hand side of this bitwise operator}}
+ // expected-note at -2 {{add parentheses after the '!' to evaluate the bitwise operator first}}
+ // expected-note at -3 {{add parentheses around left hand side expression to silence this warning}}
+ // CHECK: warn-logical-not-compare.cpp:[[line:[0-9]*]]:9: warning
+ // CHECK: to evaluate the bitwise operator first
+ // CHECK: fix-it:"{{.*}}":{[[line]]:10-[[line]]:10}:"("
+ // CHECK: fix-it:"{{.*}}":{[[line]]:15-[[line]]:15}:")"
+ // CHECK: to silence this warning
+ // CHECK: fix-it:"{{.*}}":{[[line]]:9-[[line]]:9}:"("
+ // CHECK: fix-it:"{{.*}}":{[[line]]:11-[[line]]:11}:")"
+ ret = !(x & 1);
+ ret = (!x) & 1;
+
+ // This warning is really about !x & FOO since that's a common misspelling
+ // of the negated bit test !(x & FOO). Don't warn for | and ^, since
+ // it's at least conceivable that the user wants to use | as an
+ // alternative to || that evaluates both branches. (The warning above is
+ // only emitted if the operand to ! is not a bool, but in C that's common.)
+ // And there's no logical ^.
+ ret = !x | 1;
+ ret = !(x | 1);
+ ret = (!x) | 1;
+
+ ret = !x ^ 1;
+ ret = !(x ^ 1);
+ ret = (!x) ^ 1;
+
+ // These already err, don't also warn.
+ !x &= 1; // expected-error{{expression is not assignable}}
+ !x |= 1; // expected-error{{expression is not assignable}}
+ !x ^= 1; // expected-error{{expression is not assignable}}
+
+ return ret;
+}
bool PR16673(int x) {
bool ret;
More information about the cfe-commits
mailing list