Don't warn for parentheses for the '&&' inside '||' for cases like:
assert(a || b && "bad");
since this is safe. This way we avoid a big source of such warnings which in this case are practically useless.
Note that we don't handle *all* cases where precedence wouldn't matter because of constants since
this is a bit costly to check, and IMO clarifying precedence with parentheses is good for
readability in general.
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@119533 91177308-0d34-0410-b5e6-96231b3b80d8
diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index 80ee296..17563fe 100644
--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -7329,16 +7329,56 @@
rhs->getSourceRange());
}
-static void DiagnoseLogicalAndInLogicalOr(Sema &Self, SourceLocation OpLoc,
- Expr *E) {
+/// \brief It accepts a '&&' expr that is inside a '||' one.
+/// Emit a diagnostic together with a fixit hint that wraps the '&&' expression
+/// in parentheses.
+static void
+EmitDiagnosticForLogicalAndInLogicalOr(Sema &Self, SourceLocation OpLoc,
+ Expr *E) {
+ assert(isa<BinaryOperator>(E) &&
+ cast<BinaryOperator>(E)->getOpcode() == BO_LAnd);
+ SuggestParentheses(Self, OpLoc,
+ Self.PDiag(diag::warn_logical_and_in_logical_or)
+ << E->getSourceRange(),
+ Self.PDiag(diag::note_logical_and_in_logical_or_silence),
+ E->getSourceRange(),
+ Self.PDiag(0), SourceRange());
+}
+
+/// \brief Returns true if the given expression can be evaluated as a constant
+/// 'true'.
+static bool EvaluatesAsTrue(Sema &S, Expr *E) {
+ bool Res;
+ return E->EvaluateAsBooleanCondition(Res, S.getASTContext()) && Res;
+}
+
+/// \brief Look for '&&' in the left hand of a '||' expr.
+static void DiagnoseLogicalAndInLogicalOrLHS(Sema &S, SourceLocation OpLoc,
+ Expr *E) {
if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(E)) {
if (Bop->getOpcode() == BO_LAnd) {
- SuggestParentheses(Self, OpLoc,
- Self.PDiag(diag::warn_logical_and_in_logical_or)
- << E->getSourceRange(),
- Self.PDiag(diag::note_logical_and_in_logical_or_silence),
- E->getSourceRange(),
- Self.PDiag(0), SourceRange());
+ // 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);
+ } 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()))
+ return EmitDiagnosticForLogicalAndInLogicalOr(S, OpLoc, RBop);
+ }
+ }
+ }
+}
+
+/// \brief Look for '&&' in the right hand of a '||' expr.
+static void DiagnoseLogicalAndInLogicalOrRHS(Sema &S, SourceLocation OpLoc,
+ Expr *E) {
+ if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(E)) {
+ if (Bop->getOpcode() == BO_LAnd) {
+ // 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);
}
}
}
@@ -7351,10 +7391,11 @@
if (BinaryOperator::isBitwiseOp(Opc))
return DiagnoseBitwisePrecedence(Self, Opc, OpLoc, lhs, rhs);
- /// Warn about arg1 && arg2 || arg3, as GCC 4.3+ does.
+ // Warn about arg1 || arg2 && arg3, as GCC 4.3+ does.
+ // We don't warn for 'assert(a || b && "bad")' since this is safe.
if (Opc == BO_LOr) {
- DiagnoseLogicalAndInLogicalOr(Self, OpLoc, lhs);
- DiagnoseLogicalAndInLogicalOr(Self, OpLoc, rhs);
+ DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, lhs);
+ DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, rhs);
}
}
diff --git a/test/Sema/parentheses.c b/test/Sema/parentheses.c
index a219c9b..52aadde 100644
--- a/test/Sema/parentheses.c
+++ b/test/Sema/parentheses.c
@@ -28,4 +28,10 @@
(void)(i || i && i); // expected-warning {{'&&' within '||'}} \
// expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ (void)(i || i && "w00t"); // no warning.
+ (void)("w00t" && i || i); // no warning.
+ (void)(i || i && "w00t" || i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ (void)(i || "w00t" && i || i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
}