Higuoxing updated this revision to Diff 151460.
Higuoxing added a comment.
I think I am almost there.
Here, In my sight
#define foo(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
is un-actionable, because `x op0 y op1 z` are from different arguments of
function-like macro, so, we will not check parentheses for op0 or op1 when we
call it by
foo(&&, ||, x, y, z)
but if we call it by
foo(&&, ||, x && y ||z, y, z)
it is actionable, because argument (x && y || z) is from the same arg position
of macro foo;
hence we should check `x && y || z` but shouldn't check parentheses for the
first argument (&&) and second argument (||)
I think this could cover bunch of cases. But I think my code is not so
beautiful... So, is there any suggestion?
https://reviews.llvm.org/D47687
Files:
lib/Sema/SemaExpr.cpp
test/Sema/logical-op-parentheses-in-macros.c
Index: test/Sema/logical-op-parentheses-in-macros.c
===================================================================
--- test/Sema/logical-op-parentheses-in-macros.c
+++ test/Sema/logical-op-parentheses-in-macros.c
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -Wparentheses -fsyntax-only -verify=logical_op_parentheses_check %s
+
+// operator of this case is expanded from macro function argument
+#define macro_op_parentheses_check(x) ( \
+ ( (void)(x) ) \
+)
+
+// operator of this case is expanded from macro function body
+#define macro_op_parentheses_check_ops_args(op0, op1, x, y, z) ( \
+ ( (void) (x op0 y op1 z ) ) \
+)
+
+// operator of this case is expanded from macro function body
+#define macro_op_parentheses_check_ops_args2(op0, op1, op2, w, x, y, z) ( \
+ ( (void) (w op0 x op1 y op2 z) ) \
+)
+
+// operator of this case is expanded from marco body
+#define macro_op_parentheses_check_ops_body(x, y, z) ( \
+ ( (void) (x && y || z) ) \
+)
+
+void logical_op_from_macro_arguments(unsigned i) {
+ macro_op_parentheses_check(i || i && i); // logical_op_parentheses_check-warning {{'&&' within '||'}} \
+ // logical_op_parentheses_check-note {{place parentheses around the '&&' expression to silence this warning}}
+ macro_op_parentheses_check(i || i && "I love LLVM"); // no warning.
+ macro_op_parentheses_check("I love LLVM" && i || i); // no warning.
+
+ macro_op_parentheses_check(i || i && "I love LLVM" || i); // logical_op_parentheses_check-warning {{'&&' within '||'}} \
+ // logical_op_parentheses_check-note {{place parentheses around the '&&' expression to silence this warning}}
+ macro_op_parentheses_check(i || "I love LLVM" && i || i); // logical_op_parentheses_check-warning {{'&&' within '||'}} \
+ // logical_op_parentheses_check-note {{place parentheses around the '&&' expression to silence this warning}}
+ macro_op_parentheses_check(i && i || 0); // no warning.
+ macro_op_parentheses_check(0 || i && i); // no warning.
+}
+
+void logical_op_from_macro_arguments2(unsigned i) {
+ macro_op_parentheses_check_ops_args(||, &&, i, i, i); // no warning.
+ macro_op_parentheses_check_ops_args(||, &&, i, i, "I love LLVM"); // no warning.
+ macro_op_parentheses_check_ops_args(&&, ||, "I love LLVM", i, i); // no warning.
+
+ macro_op_parentheses_check_ops_args2(||, &&, ||, i, i, "I love LLVM", i); // no warning.
+ macro_op_parentheses_check_ops_args2(||, &&, ||, i, "I love LLVM", i, i); // no warning.
+
+ macro_op_parentheses_check_ops_args(&&, ||, i, i, 0); // no warning.
+ macro_op_parentheses_check_ops_args(||, &&, 0, i, i); // no warning.
+}
+
+void logical_op_from_macro_body(unsigned i) {
+ macro_op_parentheses_check_ops_body(i, i, i); // no warning.
+}
\ No newline at end of file
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12172,7 +12172,7 @@
EmitDiagnosticForLogicalAndInLogicalOr(Sema &Self, SourceLocation OpLoc,
BinaryOperator *Bop) {
assert(Bop->getOpcode() == BO_LAnd);
- Self.Diag(Bop->getOperatorLoc(), diag::warn_logical_and_in_logical_or)
+ Self.Diag(Bop->getOperatorLoc(), diag::warn_logical_and_in_logical_or)
<< Bop->getSourceRange() << OpLoc;
SuggestParentheses(Self, Bop->getOperatorLoc(),
Self.PDiag(diag::note_precedence_silence)
@@ -12205,6 +12205,7 @@
if (EvaluatesAsFalse(S, RHSExpr))
return;
// If it's "1 && a || b" don't warn since the precedence doesn't matter.
+ // And 'assert("some message" && a || b)' don't warn as well.
if (!EvaluatesAsTrue(S, Bop->getLHS()))
return EmitDiagnosticForLogicalAndInLogicalOr(S, OpLoc, Bop);
} else if (Bop->getOpcode() == BO_LOr) {
@@ -12227,6 +12228,7 @@
if (EvaluatesAsFalse(S, LHSExpr))
return;
// If it's "a || b && 1" don't warn since the precedence doesn't matter.
+ // And 'assert(a || b && "some message")' don't warn as well.
if (!EvaluatesAsTrue(S, Bop->getRHS()))
return EmitDiagnosticForLogicalAndInLogicalOr(S, OpLoc, Bop);
}
@@ -12309,10 +12311,29 @@
}
// 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 && !OpLoc.isMacroID()/* Don't warn in macros. */) {
- DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
- DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+ // Here we will not skip 'logical and in logical or' checking
+ // in macros, since 'assert(a || b && "some message")' is equal
+ // to '(a || b && 1)' and 'assert("some message" && a || b)' is
+ // equal to '(1 && a || b)'.
+ if (Opc == BO_LOr){
+ if (!OpLoc.isMacroID()) {
+ DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
+ DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+ } else {
+ // This Expression is expanded from macro
+ SourceLocation LHSExpansionLoc, OpExpansionLoc, RHSExpansionLoc;
+ if (Self.getSourceManager().isMacroArgExpansion(OpLoc,
+ &OpExpansionLoc) &&
+ Self.getSourceManager().isMacroArgExpansion(LHSExpr->getExprLoc(),
+ &LHSExpansionLoc) &&
+ Self.getSourceManager().isMacroArgExpansion(RHSExpr->getExprLoc(),
+ &RHSExpansionLoc)) {
+ if (OpExpansionLoc == LHSExpansionLoc && OpExpansionLoc == RHSExpansionLoc) {
+ DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
+ DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+ }
+ }
+ }
}
if ((Opc == BO_Shl && LHSExpr->getType()->isIntegralType(Self.getASTContext()))
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
