hazohelet created this revision.
hazohelet added a project: clang.
Herald added a project: All.
hazohelet requested review of this revision.
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 '||'
[-Wlogical-op-parentheses]
const bool t = true;
bool tt = true || false && t; // No warning
const bool t = false;
bool tt = true || false && t; // Warning: '&&' within '||'
[-Wlogical-op-parentheses]
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.
https://reviews.llvm.org/D140860
Files:
clang/lib/AST/ExprConstant.cpp
clang/lib/Sema/SemaExpr.cpp
clang/test/Sema/logical-op-parentheses.c
clang/test/Sema/logical-op-parentheses.cpp
Index: clang/test/Sema/logical-op-parentheses.cpp
===================================================================
--- /dev/null
+++ clang/test/Sema/logical-op-parentheses.cpp
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -DSILENCE
+// RUN: %clang_cc1 -fsyntax-only -verify %s -Wlogical-op-parentheses
+// RUN: %clang_cc1 -fsyntax-only -verify %s -Wparentheses
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s -Wlogical-op-parentheses 2>&1 | FileCheck %s
+
+#ifdef SILENCE
+// expected-no-diagnostics
+#endif
+
+void logical_op_parentheses(unsigned i) {
+ constexpr bool t = 1;
+ (void)(i ||
+ i && i);
+#ifndef SILENCE
+ // expected-warning@-2 {{'&&' within '||'}}
+ // expected-note@-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}:")"
+
+ static_assert(t ||
+ t && t);
+#ifndef SILENCE
+ // expected-warning@-2 {{'&&' within '||'}}
+ // expected-note@-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}:")"
+
+ static_assert(t &&
+ t || t);
+#ifndef SILENCE
+ // expected-warning@-3 {{'&&' within '||'}}
+ // expected-note@-4 {{place parentheses around the '&&' expression to silence this warning}}
+#endif
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:17-[[@LINE-6]]:17}:"("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:15-[[@LINE-6]]:15}:")"
+
+ (void)(i || i && "w00t");
+ (void)("w00t" && i || i);
+ (void)("w00t" && t || t);
+ static_assert(t && t || 0);
+ static_assert(1 && t || t);
+ static_assert(0 || t && t);
+ static_assert(t || t && 1);
+
+ (void)(i || i && "w00t" || i);
+#ifndef SILENCE
+ // expected-warning@-2 {{'&&' within '||'}}
+ // expected-note@-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]]:26-[[@LINE-6]]:26}:")"
+
+ (void)(i || "w00t" && i || i);
+#ifndef SILENCE
+ // expected-warning@-2 {{'&&' within '||'}}
+ // expected-note@-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]]:26-[[@LINE-6]]:26}:")"
+
+ (void)(i && i || 0);
+ (void)(0 || i && i);
+}
+
Index: clang/test/Sema/logical-op-parentheses.c
===================================================================
--- clang/test/Sema/logical-op-parentheses.c
+++ 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,31 @@
// 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@-2 {{'&&' within '||'}}
+ // expected-note@-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@-3 {{'&&' within '||'}}
+ // expected-note@-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);
+ (void)(1 && t || t);
+ (void)(0 || t && t);
+ (void)(t || t && 1);
(void)(i || i && "w00t" || i);
#ifndef SILENCE
Index: clang/lib/Sema/SemaExpr.cpp
===================================================================
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15226,11 +15226,18 @@
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))
+ if (isa<IntegerLiteral, FloatingLiteral, CharacterLiteral,
+ CXXBoolLiteralExpr, CXXNullPtrLiteralExpr, FixedPointLiteral>(
+ RHSExpr) &&
+ EvaluatesAsFalse(S, RHSExpr))
return;
+ Expr *LLHS = Bop->getLHS()->IgnoreParenImpCasts();
// If it's "1 && a || b" don't warn since the precedence doesn't matter.
- if (!EvaluatesAsTrue(S, Bop->getLHS()))
- return EmitDiagnosticForLogicalAndInLogicalOr(S, OpLoc, Bop);
+ if (isa<IntegerLiteral, FloatingLiteral, CharacterLiteral,
+ CXXBoolLiteralExpr, FixedPointLiteral, StringLiteral>(LLHS) &&
+ EvaluatesAsTrue(S, LLHS))
+ return;
+ 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
@@ -15248,11 +15255,18 @@
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))
+ if (isa<IntegerLiteral, FloatingLiteral, CharacterLiteral,
+ CXXBoolLiteralExpr, CXXNullPtrLiteralExpr, FixedPointLiteral>(
+ LHSExpr) &&
+ EvaluatesAsFalse(S, LHSExpr))
return;
// If it's "a || b && 1" don't warn since the precedence doesn't matter.
- if (!EvaluatesAsTrue(S, Bop->getRHS()))
- return EmitDiagnosticForLogicalAndInLogicalOr(S, OpLoc, Bop);
+ Expr *RRHS = Bop->getRHS()->IgnoreParenImpCasts();
+ if (isa<IntegerLiteral, FloatingLiteral, CharacterLiteral,
+ CXXBoolLiteralExpr, FixedPointLiteral, StringLiteral>(RRHS) &&
+ EvaluatesAsTrue(S, RRHS))
+ return;
+ return EmitDiagnosticForLogicalAndInLogicalOr(S, OpLoc, Bop);
}
}
}
Index: clang/lib/AST/ExprConstant.cpp
===================================================================
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -15234,6 +15234,10 @@
assert(!isValueDependent() &&
"Expression evaluator can't be called on a dependent expression.");
ExprTimeTraceScope TimeScope(this, Ctx, "EvaluateAsBooleanCondition");
+ if (isa<StringLiteral>(this)) {
+ Result = true;
+ return true;
+ }
EvalResult Scratch;
return EvaluateAsRValue(Scratch, Ctx, InConstantContext) &&
HandleConversionToBool(Scratch.Val, Result);
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
