Higuoxing updated this revision to Diff 150465. Higuoxing added a comment. I step out a little further... I made an attempt to add a new warning `[-Wlogical-op-parentheses-in-macros]`. Comparing to the previous one, which do you prefer? I am new to llvm community, and as you see, this is my first patch ... so, forgive me if I made something wrong : ) suggestions are welcoming!
PS. I see something like `(x | y & z)` checking disabled in macros as well, how to deal with this? thanks https://reviews.llvm.org/D47687 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaExpr.cpp test/Sema/parentheses.c
Index: test/Sema/parentheses.c =================================================================== --- test/Sema/parentheses.c +++ test/Sema/parentheses.c @@ -14,6 +14,11 @@ if ((i = 4)) {} } +#define macro_parentheses_check(x) \ + ( \ + ( (void)(x) ) \ + ) + void bitwise_rel(unsigned i) { (void)(i & 0x2 == 0); // expected-warning {{& has lower precedence than ==}} \ // expected-note{{place parentheses around the '==' expression to silence this warning}} \ @@ -96,6 +101,21 @@ (void)(i && i || 0); // no warning. (void)(0 || i && i); // no warning. + + macro_parentheses_check(i || i && i); // expected-warning {{'&&' within '||'}} \ + // expected-note {{place parentheses around the '&&' expression to silence this warning}} + + macro_parentheses_check(i || i && "I love LLVM"); // no warning. + macro_parentheses_check("I love LLVM" && i || i); // no warning. + + macro_parentheses_check(i || i && "I love LLVM" || i); // expected-warning {{'&&' within '||'}} \ + // expected-note {{place parentheses around the '&&' expression to silence this warning}} + + macro_parentheses_check(i || "I love LLVM" && i || i); // expected-warning {{'&&' within '||'}} \ + // expected-note {{place parentheses around the '&&' expression to silence this warning}} + + macro_parentheses_check(i && i || 0); // no warning. + macro_parentheses_check(0 || i && i); // no warning. } _Bool someConditionFunc(); Index: lib/Sema/SemaExpr.cpp =================================================================== --- lib/Sema/SemaExpr.cpp +++ lib/Sema/SemaExpr.cpp @@ -12172,8 +12172,16 @@ EmitDiagnosticForLogicalAndInLogicalOr(Sema &Self, SourceLocation OpLoc, BinaryOperator *Bop) { assert(Bop->getOpcode() == BO_LAnd); - Self.Diag(Bop->getOperatorLoc(), diag::warn_logical_and_in_logical_or) + if (!OpLoc.isMacroID()) { + // if this warning is in a normal context + Self.Diag(Bop->getOperatorLoc(), diag::warn_logical_and_in_logical_or) << Bop->getSourceRange() << OpLoc; + } else { + // else this warning is in a macro context + Self.Diag(Bop->getOperatorLoc(), diag::warn_logical_and_in_logical_or_in_macros) + << Bop->getSourceRange() << OpLoc; + } + SuggestParentheses(Self, Bop->getOperatorLoc(), Self.PDiag(diag::note_precedence_silence) << Bop->getOpcodeStr(), @@ -12205,6 +12213,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 +12236,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,8 +12319,11 @@ } // 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. */) { + // 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) { DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr); DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr); } Index: include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- include/clang/Basic/DiagnosticSemaKinds.td +++ include/clang/Basic/DiagnosticSemaKinds.td @@ -5483,6 +5483,9 @@ def warn_logical_and_in_logical_or : Warning< "'&&' within '||'">, InGroup<LogicalOpParentheses>; +def warn_logical_and_in_logical_or_in_macros: Warning< + "'&&' within '||'">, InGroup<LogicalOpParenthesesInMacros>; + def warn_overloaded_shift_in_comparison :Warning< "overloaded operator %select{>>|<<}0 has higher precedence than " "comparison operator">, Index: include/clang/Basic/DiagnosticGroups.td =================================================================== --- include/clang/Basic/DiagnosticGroups.td +++ include/clang/Basic/DiagnosticGroups.td @@ -261,6 +261,7 @@ def GlobalConstructors : DiagGroup<"global-constructors">; def BitwiseOpParentheses: DiagGroup<"bitwise-op-parentheses">; def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">; +def LogicalOpParenthesesInMacros: DiagGroup<"logical-op-parentheses-in-macros">; def LogicalNotParentheses: DiagGroup<"logical-not-parentheses">; def ShiftOpParentheses: DiagGroup<"shift-op-parentheses">; def OverloadedShiftOpParentheses: DiagGroup<"overloaded-shift-op-parentheses">; @@ -668,6 +669,7 @@ def ParenthesesOnEquality : DiagGroup<"parentheses-equality">; def Parentheses : DiagGroup<"parentheses", [LogicalOpParentheses, + LogicalOpParenthesesInMacros, LogicalNotParentheses, BitwiseOpParentheses, ShiftOpParentheses,
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits