[clang] 84e3fdc - Fix -Wlogical-op-parentheses warning inconsistency for const and constexpr values
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 13 05:21:48 PST 2023
Author: Takuya Shimizu
Date: 2023-01-13T08:21:41-05:00
New Revision: 84e3fdc019412adc09b18a49063e006bd6e7efaa
URL: https://github.com/llvm/llvm-project/commit/84e3fdc019412adc09b18a49063e006bd6e7efaa
DIFF: https://github.com/llvm/llvm-project/commit/84e3fdc019412adc09b18a49063e006bd6e7efaa.diff
LOG: Fix -Wlogical-op-parentheses warning inconsistency for const and constexpr values
When using the && operator within a || operator, both Clang and GCC
produce a warning for potentially confusing operator precedence.
However, Clang avoids this warning for certain patterns, such as
a && b || 0 or a || b && 1, where the operator precedence of && and ||
does not change the result.
However, this behavior appears inconsistent when using the const or
constexpr qualifiers. For example:
bool t = true;
bool tt = true || false && t; // Warning: '&&' within '||'
const bool t = true;
bool tt = true || false && t; // No warning
const bool t = false;
bool tt = true || false && t; // Warning: '&&' within '||'
The second example does not produce a warning because
true || false && t matches the a || b && 1 pattern, while the third one
does not match any of them.
This behavior can lead to the lack of warnings for complicated
constexpr expressions. Clang should only suppress this warning when
literal values are placed in the place of t in the examples above.
This patch adds the literal-or-not check to fix the inconsistent
warnings for && within || when using const or constexpr.
Added:
Modified:
clang/docs/ReleaseNotes.rst
clang/lib/Sema/SemaExpr.cpp
clang/test/Sema/logical-op-parentheses.c
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index c5626ac8fe272..632862aaa0237 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -450,6 +450,8 @@ Improvements to Clang's diagnostics
- Clang now diagnoses overflow undefined behavior in a constant expression while
evaluating a compound assignment with remainder as operand.
- Add ``-Wreturn-local-addr``, a GCC alias for ``-Wreturn-stack-address``.
+- Clang now suppresses ``-Wlogical-op-parentheses`` on ``(x && a || b)`` and ``(a || b && x)``
+ only when ``x`` is a string literal.
Non-comprehensive list of changes in this release
-------------------------------------------------
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 2e51ec7ad7e80..93eaed1a92b1b 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -15434,38 +15434,21 @@ EmitDiagnosticForLogicalAndInLogicalOr(Sema &Self, SourceLocation OpLoc,
Bop->getSourceRange());
}
-/// Returns true if the given expression can be evaluated as a constant
-/// 'true'.
-static bool EvaluatesAsTrue(Sema &S, Expr *E) {
- bool Res;
- return !E->isValueDependent() &&
- E->EvaluateAsBooleanCondition(Res, S.getASTContext()) && Res;
-}
-
-/// Returns true if the given expression can be evaluated as a constant
-/// 'false'.
-static bool EvaluatesAsFalse(Sema &S, Expr *E) {
- bool Res;
- return !E->isValueDependent() &&
- E->EvaluateAsBooleanCondition(Res, S.getASTContext()) && !Res;
-}
-
/// Look for '&&' in the left hand of a '||' expr.
static void DiagnoseLogicalAndInLogicalOrLHS(Sema &S, SourceLocation OpLoc,
Expr *LHSExpr, Expr *RHSExpr) {
if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(LHSExpr)) {
if (Bop->getOpcode() == BO_LAnd) {
- // If it's "a && b || 0" don't warn since the precedence doesn't matter.
- if (EvaluatesAsFalse(S, RHSExpr))
- return;
- // If it's "1 && a || b" don't warn since the precedence doesn't matter.
- if (!EvaluatesAsTrue(S, Bop->getLHS()))
+ // If it's "string_literal && a || b" don't warn since the precedence
+ // doesn't matter.
+ if (!isa<StringLiteral>(Bop->getLHS()->IgnoreParenImpCasts()))
return EmitDiagnosticForLogicalAndInLogicalOr(S, OpLoc, Bop);
} else if (Bop->getOpcode() == BO_LOr) {
if (BinaryOperator *RBop = dyn_cast<BinaryOperator>(Bop->getRHS())) {
- // If it's "a || b && 1 || c" we didn't warn earlier for
- // "a || b && 1", but warn now.
- if (RBop->getOpcode() == BO_LAnd && EvaluatesAsTrue(S, RBop->getRHS()))
+ // If it's "a || b && string_literal || c" we didn't warn earlier for
+ // "a || b && string_literal", but warn now.
+ if (RBop->getOpcode() == BO_LAnd &&
+ isa<StringLiteral>(RBop->getRHS()->IgnoreParenImpCasts()))
return EmitDiagnosticForLogicalAndInLogicalOr(S, OpLoc, RBop);
}
}
@@ -15477,11 +15460,9 @@ static void DiagnoseLogicalAndInLogicalOrRHS(Sema &S, SourceLocation OpLoc,
Expr *LHSExpr, Expr *RHSExpr) {
if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(RHSExpr)) {
if (Bop->getOpcode() == BO_LAnd) {
- // If it's "0 || a && b" don't warn since the precedence doesn't matter.
- if (EvaluatesAsFalse(S, LHSExpr))
- return;
- // If it's "a || b && 1" don't warn since the precedence doesn't matter.
- if (!EvaluatesAsTrue(S, Bop->getRHS()))
+ // If it's "a || b && string_literal" don't warn since the precedence
+ // doesn't matter.
+ if (!isa<StringLiteral>(Bop->getRHS()->IgnoreParenImpCasts()))
return EmitDiagnosticForLogicalAndInLogicalOr(S, OpLoc, Bop);
}
}
diff --git a/clang/test/Sema/logical-op-parentheses.c b/clang/test/Sema/logical-op-parentheses.c
index 84464b40095fb..9a11a3e5fac3c 100644
--- a/clang/test/Sema/logical-op-parentheses.c
+++ b/clang/test/Sema/logical-op-parentheses.c
@@ -8,6 +8,7 @@
#endif
void logical_op_parentheses(unsigned i) {
+ const unsigned t = 1;
(void)(i ||
i && i);
#ifndef SILENCE
@@ -17,8 +18,55 @@ void logical_op_parentheses(unsigned i) {
// CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:14-[[@LINE-5]]:14}:"("
// CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:20-[[@LINE-6]]:20}:")"
- (void)(i || i && "w00t");
+ (void)(t ||
+ t && t);
+#ifndef SILENCE
+ // expected-warning at -2 {{'&&' within '||'}}
+ // expected-note at -3 {{place parentheses around the '&&' expression to silence this warning}}
+#endif
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:14-[[@LINE-5]]:14}:"("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:20-[[@LINE-6]]:20}:")"
+
+ (void)(t &&
+ t || t);
+#ifndef SILENCE
+ // expected-warning at -3 {{'&&' within '||'}}
+ // expected-note at -4 {{place parentheses around the '&&' expression to silence this warning}}
+#endif
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:10-[[@LINE-6]]:10}:"("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:15-[[@LINE-6]]:15}:")"
+
+ (void)(i || i && "w00t");
(void)("w00t" && i || i);
+ (void)("w00t" && t || t);
+ (void)(t && t || 0);
+#ifndef SILENCE
+ // expected-warning at -2 {{'&&' within '||'}}
+ // expected-note at -3 {{place parentheses around the '&&' expression to silence this warning}}
+#endif
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:10-[[@LINE-5]]:10}:"("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:16-[[@LINE-6]]:16}:")"
+ (void)(1 && t || t);
+#ifndef SILENCE
+ // expected-warning at -2 {{'&&' within '||'}}
+ // expected-note at -3 {{place parentheses around the '&&' expression to silence this warning}}
+#endif
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:10-[[@LINE-5]]:10}:"("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:16-[[@LINE-6]]:16}:")"
+ (void)(0 || t && t);
+#ifndef SILENCE
+ // expected-warning at -2 {{'&&' within '||'}}
+ // expected-note at -3 {{place parentheses around the '&&' expression to silence this warning}}
+#endif
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:15-[[@LINE-5]]:15}:"("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:21-[[@LINE-6]]:21}:")"
+ (void)(t || t && 1);
+#ifndef SILENCE
+ // expected-warning at -2 {{'&&' within '||'}}
+ // expected-note at -3 {{place parentheses around the '&&' expression to silence this warning}}
+#endif
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:15-[[@LINE-5]]:15}:"("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:21-[[@LINE-6]]:21}:")"
(void)(i || i && "w00t" || i);
#ifndef SILENCE
@@ -37,5 +85,17 @@ void logical_op_parentheses(unsigned i) {
// CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:26-[[@LINE-6]]:26}:")"
(void)(i && i || 0);
+#ifndef SILENCE
+ // expected-warning at -2 {{'&&' within '||'}}
+ // expected-note at -3 {{place parentheses around the '&&' expression to silence this warning}}
+#endif
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:10-[[@LINE-5]]:10}:"("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:16-[[@LINE-6]]:16}:")"
(void)(0 || i && i);
+#ifndef SILENCE
+ // expected-warning at -2 {{'&&' within '||'}}
+ // expected-note at -3 {{place parentheses around the '&&' expression to silence this warning}}
+#endif
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:15-[[@LINE-5]]:15}:"("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:21-[[@LINE-6]]:21}:")"
}
More information about the cfe-commits
mailing list