[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
vsapsai added a comment. OK. Good to know you are still working on it. In https://reviews.llvm.org/D47687#1271880, @Higuoxing wrote: > In https://reviews.llvm.org/D47687#1266893, @vsapsai wrote: > > > Sorry about the delay. The change seems to be correct but `ninja > > check-clang` reveals the test "Misc/caret-diags-macros.c" is failing. Can > > you please look into that? > > > Hi, It's because the expression is given in multiple lines and my > `SuggestParentheses` cannot give proper fix-it hint. But I could give proper > `ParenRange` in macros. Shall we reserve current `SuggestParentheses` and > just give Parenrange hit in macros ? I have no idea about this ... Why is the expression on multiple lines in this case? I didn't check in debugger but the test case looks like you can place `)` between `BAD_CONDITIONAL_OPERATOR` and `;`. At least that's what I would expect as compiler user. llvm-project/clang/test/Misc/caret-diags-macros.c:125:38: warning: operator '?:' has lower precedence than '+'; '+' will be evaluated first int test4 = BAD_CONDITIONAL_OPERATOR+BAD_CONDITIONAL_OPERATOR; ~^~~~ llvm-project/clang/test/Misc/caret-diags-macros.c:124:39: note: expanded from macro 'BAD_CONDITIONAL_OPERATOR' #define BAD_CONDITIONAL_OPERATOR (2<3)?2:3 ^ llvm-project/clang/test/Misc/caret-diags-macros.c:125:38: note: place parentheses around the '+' expression to silence this warning llvm-project/clang/test/Misc/caret-diags-macros.c:124:39: note: expanded from macro 'BAD_CONDITIONAL_OPERATOR' #define BAD_CONDITIONAL_OPERATOR (2<3)?2:3 ^ llvm-project/clang/test/Misc/caret-diags-macros.c:125:38: note: place parentheses around the '?:' expression to evaluate it first int test4 = BAD_CONDITIONAL_OPERATOR+BAD_CONDITIONAL_OPERATOR; ^ ( llvm-project/clang/test/Misc/caret-diags-macros.c:124:39: note: expanded from macro 'BAD_CONDITIONAL_OPERATOR' #define BAD_CONDITIONAL_OPERATOR (2<3)?2:3 ^ https://reviews.llvm.org/D47687 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing reclaimed this revision. Higuoxing added a comment. In https://reviews.llvm.org/D47687#1288272, @vsapsai wrote: > Sorry, you've decided to abandon the patch, it took a lot of good work. Xing, > are you sure you don't want to see this change finished? No, I am working on this :) > I agree that delays in code review can be frustrating and I think it is > something we can improve. Never mind :) Thank you, I am still working on this :) I am afraid this open revision will take up your unnecessary reviewing time or waste your reviewing time, so I close this revision temporarily ... Now, reopen this revision :) Just give me some time :) https://reviews.llvm.org/D47687 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
vsapsai added a comment. Sorry, you've decided to abandon the patch, it took a lot of good work. Xing, are you sure you don't want to see this change finished? I agree that delays in code review can be frustrating and I think it is something we can improve. https://reviews.llvm.org/D47687 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing added a comment. In https://reviews.llvm.org/D47687#1266893, @vsapsai wrote: > Sorry about the delay. The change seems to be correct but `ninja check-clang` > reveals the test "Misc/caret-diags-macros.c" is failing. Can you please look > into that? Hi, It's because the expression is given in multiple lines and my `SuggestParentheses` cannot give proper fix-it hint. But I could give proper `ParenRange` in macros. Shall we reserve current `SuggestParentheses` and just give Parenrange hit in macros ? I have no idea about this ... > Appreciate your contribution, Xing, and thanks for verifying your change with > tests. My pleasure :) Thanks for your review. https://reviews.llvm.org/D47687 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
vsapsai added a comment. Sorry about the delay. The change seems to be correct but `ninja check-clang` reveals the test "Misc/caret-diags-macros.c" is failing. Can you please look into that? Appreciate your contribution, Xing, and thanks for verifying your change with tests. Comment at: lib/Sema/SemaExpr.cpp:7107-7108 + SourceManager = Self.getSourceManager(); + SourceLocation spellLoc = SM.getSpellingLoc(ParenRange.getEnd()); + unsigned tokLen = Lexer::MeasureTokenLength(spellLoc, SM, Self.getLangOpts()); + SourceLocation EndLoc = spellLoc.getLocWithOffset(tokLen); Can you please start variables with an upper case letter? Like `SpellLoc`, `TokLen`. https://reviews.llvm.org/D47687 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing added a comment. Ping. Now, this patch could give fix-it note on parentheses in macros. https://reviews.llvm.org/D47687 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing updated this revision to Diff 166583.
Higuoxing added a comment.
I update the *SuggestParentheses* function, now, it could deal with parentheses
inside macros
https://reviews.llvm.org/D47687
Files:
lib/Sema/SemaExpr.cpp
test/Sema/parentheses.c
Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -14,7 +14,11 @@
if ((i = 4)) {}
}
-void bitwise_rel(unsigned i) {
+#define VOID_CAST(cond) ( (void)(cond) )
+#define APPLY_OPS(op0, op1, x, y, z) ( VOID_CAST(x op0 y op1 z) )
+#define APPLY_OPS_DIRECTLY(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
+void bitwise_rel(unsigned i, unsigned ) {
(void)(i & 0x2 == 0); // expected-warning {{& has lower precedence than ==}} \
// expected-note{{place parentheses around the '==' expression to silence this warning}} \
// expected-note{{place parentheses around the & expression to evaluate it first}}
@@ -96,6 +100,31 @@
(void)(i && i || 0); // no warning.
(void)(0 || i && i); // no warning.
+
+ VOID_CAST(i && i || i); // expected-warning {{'&&' within '||'}} \
+// expected-note {{place parentheses around the '&&' expression to silence this warning}}
+
+ APPLY_OPS_DIRECTLY(&&, ||, i, i, i); // no warning.
+ APPLY_OPS(&&, ||, i, i, i); // no warning.
+
+ APPLY_OPS_DIRECTLY(&&, ||, i, i, i || i && i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:41-[[@LINE-2]]:41}:"("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:47-[[@LINE-3]]:47}:")"
+
+ APPLY_OPS_DIRECTLY(&&, ||, i, i, i || i && ); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:41-[[@LINE-2]]:41}:"("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:50-[[@LINE-3]]:50}:")"
+
+ APPLY_OPS(&&, ||, i, i, i || i && i);// no warning.
+
+ APPLY_OPS_DIRECTLY(&&, ||, i, i, i && i); // no warning.
+ APPLY_OPS_DIRECTLY(&&, ||, i, i, i || i); // no warning.
+
+ APPLY_OPS(&&, ||, i, i, i && i); // no warning.
+ APPLY_OPS(&&, ||, i, i, i || i); // no warning.
+
}
_Bool someConditionFunc();
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -7103,9 +7103,11 @@
static void SuggestParentheses(Sema , SourceLocation Loc,
const PartialDiagnostic ,
SourceRange ParenRange) {
- SourceLocation EndLoc = Self.getLocForEndOfToken(ParenRange.getEnd());
- if (ParenRange.getBegin().isFileID() && ParenRange.getEnd().isFileID() &&
- EndLoc.isValid()) {
+ SourceManager = Self.getSourceManager();
+ SourceLocation spellLoc = SM.getSpellingLoc(ParenRange.getEnd());
+ unsigned tokLen = Lexer::MeasureTokenLength(spellLoc, SM, Self.getLangOpts());
+ SourceLocation EndLoc = spellLoc.getLocWithOffset(tokLen);
+ if (EndLoc.isValid()) {
Self.Diag(Loc, Note)
<< FixItHint::CreateInsertion(ParenRange.getBegin(), "(")
<< FixItHint::CreateInsertion(EndLoc, ")");
@@ -12331,6 +12333,34 @@
}
}
+static void DiagnoseLogicalAndInLogicalOr(Sema , SourceLocation OpLoc,
+ Expr *LHSExpr, Expr *RHSExpr) {
+ SourceManager = Self.getSourceManager();
+ if (!OpLoc.isMacroID()) {
+DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
+DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+return;
+ }
+
+ // Diagnose parentheses, if and only if operator and its LHS, RHS
+ // are from the same argument position of first level macros
+ SourceLocation OpExpansionLoc;
+ if (!SM.isMacroArgExpansion(OpLoc, ) ||
+ SM.getImmediateMacroCallerLoc(OpLoc).isMacroID())
+return;
+
+ SourceLocation ExprExpansionLoc;
+ if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), ) &&
+ OpExpansionLoc == ExprExpansionLoc &&
+ !SM.getImmediateMacroCallerLoc(LHSExpr->getExprLoc()).isMacroID())
+DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
+
+ if (SM.isMacroArgExpansion(RHSExpr->getExprLoc(), ) &&
+ OpExpansionLoc == ExprExpansionLoc &&
+ !SM.getImmediateMacroCallerLoc(RHSExpr->getExprLoc()).isMacroID())
+DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+}
+
/// Look for bitwise op in the left or right hand of a bitwise op with
/// lower precedence and emit a diagnostic together with a fixit hint that wraps
/// the '&' expression in parentheses.
@@ -12407,10 +12437,8 @@
// Warn about arg1 || arg2 && arg3, as GCC 4.3+ does.
// We don't warn
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing added a comment. In https://reviews.llvm.org/D47687#1154707, @vsapsai wrote: > Hope this gives you an idea how you can try to make fix-its work. Hi, Thank you @vsapsai , Yes, I am afraid if I add some extra function and may cause some problems. Because, this API `getLocForEndOfToken` seems not supported in macros. And I am not so familiar with `sourcelocation system` in clang, I will try my best to achieve this. Thanks a lot https://reviews.llvm.org/D47687 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
vsapsai added a comment.
In https://reviews.llvm.org/D47687#1136351, @Higuoxing wrote:
> Sorry, It seems a little bit difficult for me to add a proper fix-it hint for
> expressions in macros, because I cannot find the exact position of the last
> char of the token on right hand side of operator. Any suggestion?
Can you please clarify what exactly isn't working? `SuggestParentheses` is
doing most of what you need but in this case it doesn't work because of macro
locations:
static void SuggestParentheses(Sema , SourceLocation Loc,
const PartialDiagnostic ,
SourceRange ParenRange) {
SourceLocation EndLoc = Self.getLocForEndOfToken(ParenRange.getEnd());
if (ParenRange.getBegin().isFileID() && ParenRange.getEnd().isFileID() &&
EndLoc.isValid()) {
Self.Diag(Loc, Note)
<< FixItHint::CreateInsertion(ParenRange.getBegin(), "(")
<< FixItHint::CreateInsertion(EndLoc, ")");
} else {
// We can't display the parentheses, so just show the bare note.
Self.Diag(Loc, Note) << ParenRange;
}
}
Hope this gives you an idea how you can try to make fix-its work.
https://reviews.llvm.org/D47687
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing added a comment.
Sorry, It seems a little bit difficult for me to add a proper fix-it hint for
expressions in macros, because I cannot find the exact position of the last
char of the token on right hand side of operator. Any suggestion?
Actually, in gcc, it will emit warning for everything without a fix-it hint,
let alone expressions in nested macros... I think that our warning note is
enough for us to diagnose the lacking parentheses, as you see in my inline
comment
llvm/tools/clang/test/Sema/parentheses.c:109:15: note: place parentheses
around the '&&' expression to silence this warning
VOID_CAST(i && i || i); // expected-warning {{'&&' within '||'}} \
~~^~~~
llvm/tools/clang/test/Sema/parentheses.c:17:34: note: expanded from macro
'VOID_CAST'
#define VOID_CAST(cond) ( (void)(cond) )
^~~~
So, it depends on you whether the parentheses checking in macros should be
reserved. Both OK for me... Thanks for your reviewing :)
https://reviews.llvm.org/D47687
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing updated this revision to Diff 151736.
Higuoxing added a comment.
test case has been passed
https://reviews.llvm.org/D47687
Files:
lib/Sema/SemaExpr.cpp
test/Sema/parentheses.c
Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -14,6 +14,10 @@
if ((i = 4)) {}
}
+#define VOID_CAST(cond) ( (void)(cond) )
+#define APPLY_OPS(op0, op1, x, y, z) ( VOID_CAST(x op0 y op1 z) )
+#define APPLY_OPS_DIRECTLY(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
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 +100,22 @@
(void)(i && i || 0); // no warning.
(void)(0 || i && i); // no warning.
+
+ VOID_CAST(i && i || i); // expected-warning {{'&&' within '||'}} \
+// expected-note {{place parentheses around the
'&&' expression to silence this warning}}
+
+ APPLY_OPS_DIRECTLY(&&, ||, i, i, i); // no warning.
+ APPLY_OPS(&&, ||, i, i, i); // no warning.
+
+ APPLY_OPS_DIRECTLY(&&, ||, i, i, i || i && i); // expected-warning {{'&&'
within '||'}} \
+ // expected-note {{place
parentheses around the '&&' expression to silence this warning}}
+ APPLY_OPS(&&, ||, i, i, i || i && i);// no warning.
+
+ APPLY_OPS_DIRECTLY(&&, ||, i, i, i && i); // no warning.
+ APPLY_OPS_DIRECTLY(&&, ||, i, i, i || i); // no warning.
+
+ APPLY_OPS(&&, ||, i, i, i && i); // no warning.
+ APPLY_OPS(&&, ||, i, i, i || i); // no warning.
}
_Bool someConditionFunc();
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12233,6 +12233,35 @@
}
}
+/// Look for '&&' in the righ or left hand side of a '||' expr
+static void DiagnoseLogicalAndInLogicalOr(Sema , SourceLocation OpLoc,
+ Expr *LHSExpr, Expr *RHSExpr) {
+ SourceManager = Self.getSourceManager();
+ if (!OpLoc.isMacroID()) {
+DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
+DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+return;
+ }
+
+ // Diagnose parentheses, if and only if operator and its LHS, RHS
+ // are from the same argument position of first level macros
+ SourceLocation OpExpansionLoc;
+ if (!SM.isMacroArgExpansion(OpLoc, ) ||
+ SM.getImmediateMacroCallerLoc(OpLoc).isMacroID())
+return;
+
+ SourceLocation ExprExpansionLoc;
+ if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), ) &&
+ OpExpansionLoc == ExprExpansionLoc &&
+ !SM.getImmediateMacroCallerLoc(LHSExpr->getExprLoc()).isMacroID())
+DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
+
+ if (SM.isMacroArgExpansion(RHSExpr->getExprLoc(), ) &&
+ OpExpansionLoc == ExprExpansionLoc &&
+ !SM.getImmediateMacroCallerLoc(RHSExpr->getExprLoc()).isMacroID())
+DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+}
+
/// Look for bitwise op in the left or right hand of a bitwise op with
/// lower precedence and emit a diagnostic together with a fixit hint that
wraps
/// the '&' expression in parentheses.
@@ -12310,10 +12339,8 @@
// 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);
- }
+ if (Opc == BO_LOr)
+DiagnoseLogicalAndInLogicalOr(Self, OpLoc, LHSExpr, RHSExpr);
if ((Opc == BO_Shl &&
LHSExpr->getType()->isIntegralType(Self.getASTContext()))
|| Opc == BO_Shr) {
Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -14,6 +14,10 @@
if ((i = 4)) {}
}
+#define VOID_CAST(cond) ( (void)(cond) )
+#define APPLY_OPS(op0, op1, x, y, z) ( VOID_CAST(x op0 y op1 z) )
+#define APPLY_OPS_DIRECTLY(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
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 +100,22 @@
(void)(i && i || 0); // no warning.
(void)(0 || i && i); // no warning.
+
+ VOID_CAST(i && i || i); // expected-warning {{'&&' within '||'}} \
+// expected-note {{place parentheses around the '&&' expression to silence this warning}}
+
+ APPLY_OPS_DIRECTLY(&&, ||, i, i, i); // no warning.
+ APPLY_OPS(&&, ||, i, i, i);
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing added a comment. In https://reviews.llvm.org/D47687#1135284, @dexonsmith wrote: > Did you miss this comment from my previous review? > > > I would like to see tests like the following: > > > > NESTING_VOID_WRAPPER(&&, ||, i, i, i && i); // no warning. > > NESTING_VOID_WRAPPER(||, &&, i, i, i || i); // no warning. I think this test case should pass, please wait I cloned a newer version codes, and I am compiling it... about 1-2 hours Sorry for disturbing you too much on reviewing codes I think I could give it a try on giving it a fix-it... Big thanks! https://reviews.llvm.org/D47687 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
dexonsmith added a comment.
Did you miss this comment from my previous review?
> I would like to see tests like the following:
>
> NESTING_VOID_WRAPPER(&&, ||, i, i, i && i); // no warning.
> NESTING_VOID_WRAPPER(||, &&, i, i, i || i); // no warning.
Comment at: lib/Sema/SemaExpr.cpp:12254-12255
+if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), ) &&
+!SM.getImmediateMacroCallerLoc(LHSExpr->getExprLoc()).isMacroID() &&
+OpExpansionLoc == ExprExpansionLoc)
+ DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
Can you reverse these two checks? The second one looks cheaper.
Comment at: lib/Sema/SemaExpr.cpp:12243
+DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+ } else {
+SourceLocation OpExpansionLoc;
dexonsmith wrote:
> You can reduce nesting below by adding an early return above this.
>
> I also think you should describe at a high-level what you're trying to do in
> the code that follows. Something like:
> ```
> // Only diagnose operators in macros if they're from the same argument
> position.
> ```
You added an early return -- but then you didn't actually reduce the nesting.
Please remove the else that follows.
Comment at: test/Sema/parentheses.c:114
+ NON_NESTING_VOID_0(i && i || i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around
the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_0((i && i) || i); // no warning.
Higuoxing wrote:
> Higuoxing wrote:
> > dexonsmith wrote:
> > > Can you add fix-it CHECKs?
> > ```
> > llvm/tools/clang/test/Sema/parentheses.c:109:15: note: place parentheses
> > around the '&&' expression to silence this warning
> > VOID_CAST(i && i || i); // expected-warning {{'&&' within '||'}} \
> > ~~^~~~
> > llvm/tools/clang/test/Sema/parentheses.c:17:34: note: expanded from macro
> > 'VOID_CAST'
> > #define VOID_CAST(cond) ( (void)(cond) )
> > ^~~~
> > ```
> >
> > Sorry, it seems that when deal with expressions in macros, there is no
> > fix-it hint ...
> The `suggestParentheses` suppress the fix-it hint when the expression is in
> macros
>
> ```
> if (ParenRange.getBegin().isFileID() && ParenRange.getEnd().isFileID() &&
> EndLoc.isValid()) {
> Self.Diag(Loc, Note)
> << FixItHint::CreateInsertion(ParenRange.getBegin(), "(")
> << FixItHint::CreateInsertion(EndLoc, ")");
> } else {
> // We can't display the parentheses, so just show the bare note.
> Self.Diag(Loc, Note) << ParenRange;
> }
> ```
>
> You see, there is a `isFileID()`
Can you make it work? The diagnostic was disabled because it was low quality:
no fix-it, and firing when it was not actionable. I'm not convinced we should
turn it back on until we can give a fix-it.
https://reviews.llvm.org/D47687
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing updated this revision to Diff 151661.
Higuoxing marked 12 inline comments as done.
Higuoxing removed a reviewer: echristo.
https://reviews.llvm.org/D47687
Files:
lib/Sema/SemaExpr.cpp
test/Sema/parentheses.c
Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -14,6 +14,10 @@
if ((i = 4)) {}
}
+#define VOID_CAST(cond) ( (void)(cond) )
+#define APPLY_OPS(op0, op1, x, y, z) ( VOID_CAST(x op0 y op1 z) )
+#define APPLY_OPS_DIRECTLY(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
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 +100,18 @@
(void)(i && i || 0); // no warning.
(void)(0 || i && i); // no warning.
+
+ VOID_CAST(i && i || i); // expected-warning {{'&&' within '||'}} \
+// expected-note {{place parentheses around the
'&&' expression to silence this warning}}
+ VOID_CAST((i && i) || i); // no warning.
+
+ APPLY_OPS_DIRECTLY(&&, ||, i, i, i); // no warning.
+ APPLY_OPS(&&, ||, i, i, i); // no warning.
+
+ APPLY_OPS_DIRECTLY(&&, ||, i, i, i || i && i); // expected-warning {{'&&'
within '||'}} \
+ // expected-note {{place
parentheses around the '&&' expression to silence this warning}}
+ APPLY_OPS_DIRECTLY(&&, ||, i, i, i || (i && i)); // no warning.
+ APPLY_OPS(&&, ||, i, i, i || i && i);// no warning.
}
_Bool someConditionFunc();
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12233,6 +12233,35 @@
}
}
+/// Look for '&&' in the righ or left hand side of a '||' expr
+static void DiagnoseLogicalAndInLogicalOr(Sema , SourceLocation OpLoc,
+ Expr *LHSExpr, Expr *RHSExpr) {
+ SourceManager = Self.getSourceManager();
+ if (!OpLoc.isMacroID()) {
+DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
+DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+return;
+ } else {
+// Diagnose parentheses, if and only if operator and its LHS, RHS
+// are from the same argument position of first level macros
+SourceLocation OpExpansionLoc;
+if (!SM.isMacroArgExpansion(OpLoc, ) ||
+SM.getImmediateMacroCallerLoc(OpLoc).isMacroID())
+ return;
+
+SourceLocation ExprExpansionLoc;
+if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), ) &&
+!SM.getImmediateMacroCallerLoc(LHSExpr->getExprLoc()).isMacroID() &&
+OpExpansionLoc == ExprExpansionLoc)
+ DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
+
+if (SM.isMacroArgExpansion(RHSExpr->getExprLoc(), ) &&
+!SM.getImmediateMacroCallerLoc(LHSExpr->getExprLoc()).isMacroID() &&
+OpExpansionLoc == ExprExpansionLoc)
+ DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+ }
+}
+
/// Look for bitwise op in the left or right hand of a bitwise op with
/// lower precedence and emit a diagnostic together with a fixit hint that
wraps
/// the '&' expression in parentheses.
@@ -12310,10 +12339,8 @@
// 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);
- }
+ if (Opc == BO_LOr)
+DiagnoseLogicalAndInLogicalOr(Self, OpLoc, LHSExpr, RHSExpr);
if ((Opc == BO_Shl &&
LHSExpr->getType()->isIntegralType(Self.getASTContext()))
|| Opc == BO_Shr) {
Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -14,6 +14,10 @@
if ((i = 4)) {}
}
+#define VOID_CAST(cond) ( (void)(cond) )
+#define APPLY_OPS(op0, op1, x, y, z) ( VOID_CAST(x op0 y op1 z) )
+#define APPLY_OPS_DIRECTLY(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
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 +100,18 @@
(void)(i && i || 0); // no warning.
(void)(0 || i && i); // no warning.
+
+ VOID_CAST(i && i || i); // expected-warning {{'&&' within '||'}} \
+// expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ VOID_CAST((i && i) || i); // no warning.
+
+ APPLY_OPS_DIRECTLY(&&, ||, i, i, i); // no warning.
+ APPLY_OPS(&&, ||, i, i, i);
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing added inline comments.
Comment at: test/Sema/parentheses.c:114
+ NON_NESTING_VOID_0(i && i || i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around
the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_0((i && i) || i); // no warning.
Higuoxing wrote:
> dexonsmith wrote:
> > Can you add fix-it CHECKs?
> ```
> llvm/tools/clang/test/Sema/parentheses.c:109:15: note: place parentheses
> around the '&&' expression to silence this warning
> VOID_CAST(i && i || i); // expected-warning {{'&&' within '||'}} \
> ~~^~~~
> llvm/tools/clang/test/Sema/parentheses.c:17:34: note: expanded from macro
> 'VOID_CAST'
> #define VOID_CAST(cond) ( (void)(cond) )
> ^~~~
> ```
>
> Sorry, it seems that when deal with expressions in macros, there is no fix-it
> hint ...
The `suggestParentheses` suppress the fix-it hint when the expression is in
macros
```
if (ParenRange.getBegin().isFileID() && ParenRange.getEnd().isFileID() &&
EndLoc.isValid()) {
Self.Diag(Loc, Note)
<< FixItHint::CreateInsertion(ParenRange.getBegin(), "(")
<< FixItHint::CreateInsertion(EndLoc, ")");
} else {
// We can't display the parentheses, so just show the bare note.
Self.Diag(Loc, Note) << ParenRange;
}
```
You see, there is a `isFileID()`
https://reviews.llvm.org/D47687
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing updated this revision to Diff 151654.
Higuoxing marked an inline comment as done.
https://reviews.llvm.org/D47687
Files:
lib/Sema/SemaExpr.cpp
test/Sema/parentheses.c
Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -14,6 +14,10 @@
if ((i = 4)) {}
}
+#define VOID_CAST(cond) ( (void)(cond) )
+#define APPLY_OPS(op0, op1, x, y, z) ( VOID_CAST(x op0 y op1 z) )
+#define APPLY_OPS_DIRECTLY(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
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 +100,13 @@
(void)(i && i || 0); // no warning.
(void)(0 || i && i); // no warning.
+
+ VOID_CAST(i && i || i); // expected-warning {{'&&' within '||'}} \
+// expected-note {{place parentheses around the
'&&' expression to silence this warning}}
+ VOID_CAST((i && i) || i); // no warning.
+
+ APPLY_OPS_DIRECTLY(&&, ||, i, i, i); // no warning.
+ APPLY_OPS(&&, ||, i, i, i); // no warning.
}
_Bool someConditionFunc();
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12233,6 +12233,35 @@
}
}
+/// Look for '&&' in the righ or left hand of a '||' expr
+static void DiagnoseLogicalAndInLogicalOr(Sema , SourceLocation OpLoc,
+ Expr *LHSExpr, Expr *RHSExpr) {
+ SourceManager = Self.getSourceManager();
+ if (!OpLoc.isMacroID()) {
+DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
+DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+return;
+ } else {
+// Diagnose parentheses if and only if operator and its LHS, RHS
+// are from the same argument position of first level macros
+SourceLocation OpExpansionLoc;
+if (!SM.isMacroArgExpansion(OpLoc, ) ||
+SM.getImmediateMacroCallerLoc(OpLoc).isMacroID())
+ return;
+
+SourceLocation ExprExpansionLoc;
+if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), ) &&
+!SM.getImmediateMacroCallerLoc(LHSExpr->getExprLoc()).isMacroID() &&
+OpExpansionLoc == ExprExpansionLoc)
+ DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
+
+if (SM.isMacroArgExpansion(RHSExpr->getExprLoc(), ) &&
+!SM.getImmediateMacroCallerLoc(LHSExpr->getExprLoc()).isMacroID() &&
+OpExpansionLoc == ExprExpansionLoc)
+ DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+ }
+}
+
/// Look for bitwise op in the left or right hand of a bitwise op with
/// lower precedence and emit a diagnostic together with a fixit hint that
wraps
/// the '&' expression in parentheses.
@@ -12310,10 +12339,8 @@
// 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);
- }
+ if (Opc == BO_LOr)
+DiagnoseLogicalAndInLogicalOr(Self, OpLoc, LHSExpr, RHSExpr);
if ((Opc == BO_Shl &&
LHSExpr->getType()->isIntegralType(Self.getASTContext()))
|| Opc == BO_Shr) {
Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -14,6 +14,10 @@
if ((i = 4)) {}
}
+#define VOID_CAST(cond) ( (void)(cond) )
+#define APPLY_OPS(op0, op1, x, y, z) ( VOID_CAST(x op0 y op1 z) )
+#define APPLY_OPS_DIRECTLY(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
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 +100,13 @@
(void)(i && i || 0); // no warning.
(void)(0 || i && i); // no warning.
+
+ VOID_CAST(i && i || i); // expected-warning {{'&&' within '||'}} \
+// expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ VOID_CAST((i && i) || i); // no warning.
+
+ APPLY_OPS_DIRECTLY(&&, ||, i, i, i); // no warning.
+ APPLY_OPS(&&, ||, i, i, i); // no warning.
}
_Bool someConditionFunc();
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12233,6 +12233,35 @@
}
}
+/// Look for '&&' in the righ or left hand of a '||' expr
+static void DiagnoseLogicalAndInLogicalOr(Sema , SourceLocation OpLoc,
+ Expr
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing marked 9 inline comments as done.
Higuoxing added inline comments.
Comment at: test/Sema/parentheses.c:20
+#define NESTING_VOID(cond) ( (void)(cond) )
+#define NESTING_VOID_WRAPPER(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
dexonsmith wrote:
> - You haven't actually wrapped `NESTING_VOID` here.
> - A more descriptive name would be `APPLY_OPS` or something.
Yes, I made a crucial mistake! So, I make a little adjustment here, we will
check parentheses for expressions, if and only if the operator and its LHS, RHS
are from same arg position of a *non-nesting macro*, because it seems very
difficult to distinguish a nesting macro's args are from same arg position of
its `father` macro
Comment at: test/Sema/parentheses.c:109-111
+ //===--
+ // Logical operator in macros
+ //===--
dexonsmith wrote:
> I'm not sure this comment is particularly useful.
Yes, I delete them : )
Comment at: test/Sema/parentheses.c:114
+ NON_NESTING_VOID_0(i && i || i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around
the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_0((i && i) || i); // no warning.
dexonsmith wrote:
> Can you add fix-it CHECKs?
```
llvm/tools/clang/test/Sema/parentheses.c:109:15: note: place parentheses around
the '&&' expression to silence this warning
VOID_CAST(i && i || i); // expected-warning {{'&&' within '||'}} \
~~^~~~
llvm/tools/clang/test/Sema/parentheses.c:17:34: note: expanded from macro
'VOID_CAST'
#define VOID_CAST(cond) ( (void)(cond) )
^~~~
```
Sorry, it seems that when deal with expressions in macros, there is no fix-it
hint ...
Comment at: test/Sema/parentheses.c:117-124
+ // NON_NESTING_VOID_1(&&, ||, i, i, i);
+ // will be expanded to:
+ // i && i || i
+ // ^ arg position 2 (i)
+ //^ arg position 0 (&&)
+ // ^ arg position 3 (||) should not be checked becaues `op ||` and
nothing from same arg position
+ // ^ arg position 1 (i)
dexonsmith wrote:
> I think this comment should be fairly well implied by the commit and commit
> message the test is part of. I don't think it's necessary.
Yes, I delete them
https://reviews.llvm.org/D47687
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
dexonsmith added inline comments.
Comment at: lib/Sema/SemaExpr.cpp:12236
+/// Look for '&&' in the righ or left hand of a '||' expr
+static void DiagnoseLogicalAndInLogicalOr(Sema , SourceLocation OpLoc,
- Please add a period at the end of the sentence.
- "righ" should be "right".
Comment at: lib/Sema/SemaExpr.cpp:12243
+DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+ } else {
+SourceLocation OpExpansionLoc;
You can reduce nesting below by adding an early return above this.
I also think you should describe at a high-level what you're trying to do in
the code that follows. Something like:
```
// Only diagnose operators in macros if they're from the same argument position.
```
Comment at: lib/Sema/SemaExpr.cpp:12249
+SourceLocation ExprExpansionLoc;
+// LHS and operator are from same arg position of macro function
+if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), ) &&
This comment is just describing what's clear from the code. I think you should
drop it, and the similar one later.
Comment at: lib/Sema/SemaExpr.cpp:12251
+if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), ) &&
+OpExpansionLoc == ExprExpansionLoc)
+ DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
This line has odd indentation. Please run clang-format-diff.py to clean up the
patch.
Comment at: test/Sema/parentheses.c:17-18
+// testing macros
+// nesting macros testing
+#define NESTING_VOID(cond) ( (void)(cond) )
I don't think these comments are useful.
Comment at: test/Sema/parentheses.c:19
+// nesting macros testing
+#define NESTING_VOID(cond) ( (void)(cond) )
+#define NESTING_VOID_WRAPPER(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
Can you combine this with `NON_NESTING_VOID_0` (which I think should be called
`VOID_CAST`) below?
Comment at: test/Sema/parentheses.c:20
+#define NESTING_VOID(cond) ( (void)(cond) )
+#define NESTING_VOID_WRAPPER(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
- You haven't actually wrapped `NESTING_VOID` here.
- A more descriptive name would be `APPLY_OPS` or something.
Comment at: test/Sema/parentheses.c:23
+// non-nesting macros
+#define NON_NESTING_VOID_0(cond) ( (void)(cond) )
+#define NON_NESTING_VOID_1(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
This name is strange. `VOID_CAST` would be more descriptive.
Comment at: test/Sema/parentheses.c:24
+#define NON_NESTING_VOID_0(cond) ( (void)(cond) )
+#define NON_NESTING_VOID_1(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
I suggest `APPLY_OPS_DIRECTLY`.
Comment at: test/Sema/parentheses.c:109-111
+ //===--
+ // Logical operator in macros
+ //===--
I'm not sure this comment is particularly useful.
Comment at: test/Sema/parentheses.c:114
+ NON_NESTING_VOID_0(i && i || i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around
the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_0((i && i) || i); // no warning.
Can you add fix-it CHECKs?
Comment at: test/Sema/parentheses.c:117-124
+ // NON_NESTING_VOID_1(&&, ||, i, i, i);
+ // will be expanded to:
+ // i && i || i
+ // ^ arg position 2 (i)
+ //^ arg position 0 (&&)
+ // ^ arg position 3 (||) should not be checked becaues `op ||` and
nothing from same arg position
+ // ^ arg position 1 (i)
I think this comment should be fairly well implied by the commit and commit
message the test is part of. I don't think it's necessary.
Comment at: test/Sema/parentheses.c:140
+
+ // same as this one
+ NESTING_VOID_WRAPPER(&&, ||, i && i || i, i, i); // expected-warning {{'&&'
within '||'}} \
Not a useful comment.
Comment at: test/Sema/parentheses.c:153
+ // ^ arg position 4 (i)
+ NON_NESTING_VOID_1(&&, ||, i, i && i || i, i); // expected-warning {{'&&'
within '||'}} \
+ // expected-note {{place
parentheses around the '&&' expression to silence this warning}}
I don't think checking within other macro arguments is necessary here. You
have a combinatorial explosion of tests, but it seems unlikely code would be
written in such a way as to make this wrong.
I would like to see tests like the following:
```
NESTING_VOID_WRAPPER(&&, ||, i, i, i && i); // no warning.
NESTING_VOID_WRAPPER(||, &&, i, i, i || i);
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing added inline comments.
Comment at: lib/Sema/SemaExpr.cpp:12304-12309
// Diagnose "arg1 & arg2 | arg3"
if ((Opc == BO_Or || Opc == BO_Xor) &&
!OpLoc.isMacroID()/* Don't warn in macros. */) {
DiagnoseBitwiseOpInBitwiseOp(Self, Opc, OpLoc, LHSExpr);
DiagnoseBitwiseOpInBitwiseOp(Self, Opc, OpLoc, RHSExpr);
}
dexonsmith wrote:
> It seems unfortunate not to update this one at the same time. Or are you
> planning that for a follow-up?
>
> Can you think of a way to share the logic cleanly?
Yes, I would like to update it after this patch being accepted, because I think
this patch is about `logical-op-parentheses` and this one is about
`bitwise-op`, and I think after this patch being accepted I will be more
confident on the code style, API using and various things : ) Of course, I
would like to help!
Comment at: lib/Sema/SemaExpr.cpp:12313
// 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);
+ if (Opc == BO_LOr) {
+if (!OpLoc.isMacroID()) {
dexonsmith wrote:
> I think the code inside this should be split out into a separate function
> (straw man name: "diagnoseLogicalAndInLogicalOr") so that you can use early
> returns and reduce nesting.
Yes, I add a function `DiagnoseLogicalAndInLogicalOr`, does the uppercase `D`
matter?
Comment at: lib/Sema/SemaExpr.cpp:12319
+} else {
+ // This Expression is expanded from macro
+ SourceLocation LHSExpansionLoc, OpExpansionLoc, RHSExpansionLoc;
dexonsmith wrote:
> This comment is just repeating what's in the condition. I suggest replacing
> it with something describing the logic that follows. Also, it's missing a
> period at the end of the sentence.
Sorry, what period is missing?
Comment at: lib/Sema/SemaExpr.cpp:12325-12328
+ (Self.getSourceManager().isMacroArgExpansion(OpLoc,
+ ) &&
+ Self.getSourceManager().isMacroArgExpansion(RHSExpr->getExprLoc(),
+ ))) {
dexonsmith wrote:
> It's a little awkward to add this condition in with the previous one, and
> you're also repeating a call to `isMacroArgExpansion` unnecessarily. I
> suggest something like:
>
> ```
> SourceLocation OpExpansion;
> if (!SM.isMacroArgExpansion(OpLoc, ))
> return;
>
> SourceLocation ExprExpansion;
> if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), ) &&
> OpExpansion == ExprExpansion)
> DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
>
> if (SM.isMacroArgExpansion(RHSExpr->getExprLoc(), ) &&
> OpExpansion == ExprExpansion)
> DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
> ```
>
Great! The code is more elegant!
https://reviews.llvm.org/D47687
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing updated this revision to Diff 151605.
https://reviews.llvm.org/D47687
Files:
lib/Sema/SemaExpr.cpp
test/Sema/parentheses.c
Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -14,6 +14,15 @@
if ((i = 4)) {}
}
+// testing macros
+// nesting macros testing
+#define NESTING_VOID(cond) ( (void)(cond) )
+#define NESTING_VOID_WRAPPER(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
+// non-nesting macros
+#define NON_NESTING_VOID_0(cond) ( (void)(cond) )
+#define NON_NESTING_VOID_1(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
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 +105,74 @@
(void)(i && i || 0); // no warning.
(void)(0 || i && i); // no warning.
+
+ //===--
+ // Logical operator in macros
+ //===--
+
+ // actionable expression tests
+ NON_NESTING_VOID_0(i && i || i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_0((i && i) || i); // no warning.
+
+ // NON_NESTING_VOID_1(&&, ||, i && i || i, i, i);
+ // will be expanded to:
+ // i && i || i && i || i
+ // ^-^ arg position 2 (i && i || i) should be checked because `op ||` and `lhs (i && i)` from same arg position
+ // ^ arg position 0 (&&)
+ //^ arg position 3 (i)
+ // ^ arg position 1 (||) should not be checked because `op ||` and nothing from same arg position
+ // ^ arg position 4 (i)
+ NON_NESTING_VOID_1(&&, ||, i && i || i, i, i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_1(&&, ||, ((i && i) || i), i, i); // no warning.
+
+ // same as this one
+ NESTING_VOID_WRAPPER(&&, ||, i && i || i, i, i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NESTING_VOID_WRAPPER(&&, ||, ((i && i) || i), i, i); // no waring.
+
+ // NON_NESTING_VOID_1(&&, ||, i, i && i || i, i);
+ // will be expanded to:
+ // i && i && i || i || i
+ // ^ arg position 2 (i)
+ //^ arg position 0 (&&)
+ // ^-^ arg position 3 (i && i || i) should be checked because `op ||` and `lhs i && i` from same arg position
+ // ^ arg position 1 (||) should not be checked because `op ||` and nothing from same arg postion
+ // ^ arg position 4 (i)
+ NON_NESTING_VOID_1(&&, ||, i, i && i || i, i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_1(&&, ||, i, (i && i) || i, i); // no warning.
+
+ // same as this one
+ NESTING_VOID_WRAPPER(&&, ||, i, i && i || i, i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NESTING_VOID_WRAPPER(&&, ||, i, (i && i) || i, i); // no warning.
+
+ // un-actionable expression tests
+ // NON_NESTING_VOID_1(&&, ||, i, i, i);
+ // will be expanded to:
+ // i && i || i
+ // ^ arg position 2 (i)
+ //^ arg position 0 (&&)
+ // ^ arg position 3 (||) should not be checked becaues `op ||` and nothing from same arg position
+ // ^ arg position 1 (i)
+ // ^ arg position 4 (i)
+ NON_NESTING_VOID_1(&&, ||, i, i, i); // no warning.
+ NESTING_VOID_WRAPPER(&&, ||, i, i, i); // no warning.
+
+ // NON_NESTING_VOID_1(&&, ||, i, i, i && i || i);
+ // will be expanded to:
+ // i && i || i && i || i
+ // ^ arg positon 2 (i)
+ //^ arg position 0 (&&)
+ // ^ arg position 3 (i)
+ // ^ arg position 1 (||) should not be checked because `op ||` and nothing from same arg position
+ // ^-^ arg position 4 (i && i || i) should not be checked because `op ||` and nothing from same arg position
+ NON_NESTING_VOID_1(&&, ||, i, i, i && i || i); // no warning.
+ NESTING_VOID_WRAPPER(&&, ||, i, i, i && i || i); // no warning.
+
+ // same for something expression like (i || i && i);
}
_Bool someConditionFunc();
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12310,9 +12310,30 @@
//
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing updated this revision to Diff 151604.
Higuoxing added a comment.
Ping, thanks
https://reviews.llvm.org/D47687
Files:
lib/Sema/SemaExpr.cpp
test/Sema/parentheses.c
Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -14,6 +14,15 @@
if ((i = 4)) {}
}
+// testing macros
+// nesting macros testing
+#define NESTING_VOID(cond) ( (void)(cond) )
+#define NESTING_VOID_WRAPPER(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
+// non-nesting macros
+#define NON_NESTING_VOID_0(cond) ( (void)(cond) )
+#define NON_NESTING_VOID_1(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
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 +105,78 @@
(void)(i && i || 0); // no warning.
(void)(0 || i && i); // no warning.
+
+ //===--
+ // Logical operator in macros
+ //===--
+
+ // actionable expression tests
+ NON_NESTING_VOID_0(i && i || i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_0((i && i) || i); // no warning.
+
+ // NON_NESTING_VOID_1(&&, ||, i && i || i, i, i);
+ // will be expanded to:
+ // i && i || i && i || i
+ // ^-^ arg position 2 (i && i || i) should be checked because `op ||` and `lhs (i && i)` from same arg position
+ // ^ arg position 0 (&&)
+ //^ arg position 3 (i)
+ // ^ arg position 1 (||) should not be checked because `op ||` and nothing from same arg position
+ // ^ arg position 4 (i)
+ NON_NESTING_VOID_1(&&, ||, i && i || i, i, i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}} \
+ // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_1(&&, ||, ((i && i) || i), i, i); // no warning.
+
+ // same as this one
+ NESTING_VOID_WRAPPER(&&, ||, i && i || i, i, i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}} \
+ // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NESTING_VOID_WRAPPER(&&, ||, ((i && i) || i), i, i); // no waring.
+
+ // NON_NESTING_VOID_1(&&, ||, i, i && i || i, i);
+ // will be expanded to:
+ // i && i && i || i || i
+ // ^ arg position 2 (i)
+ //^ arg position 0 (&&)
+ // ^-^ arg position 3 (i && i || i) should be checked because `op ||` and `lhs i && i` from same arg position
+ // ^ arg position 1 (||) should not be checked because `op ||` and nothing from same arg postion
+ // ^ arg position 4 (i)
+ NON_NESTING_VOID_1(&&, ||, i, i && i || i, i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_1(&&, ||, i, (i && i) || i, i); // no warning.
+
+ // same as this one
+ NESTING_VOID_WRAPPER(&&, ||, i, i && i || i, i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NESTING_VOID_WRAPPER(&&, ||, i, (i && i) || i, i); // no warning.
+
+ // un-actionable expression tests
+ // NON_NESTING_VOID_1(&&, ||, i, i, i);
+ // will be expanded to:
+ // i && i || i
+ // ^ arg position 2 (i)
+ //^ arg position 0 (&&)
+ // ^ arg position 3 (||) should not be checked becaues `op ||` and nothing from same arg position
+ // ^ arg position 1 (i)
+ // ^ arg position 4 (i)
+ NON_NESTING_VOID_1(&&, ||, i, i, i); // no warning.
+ NESTING_VOID_WRAPPER(&&, ||, i, i, i); // no warning.
+
+ // NON_NESTING_VOID_1(&&, ||, i, i, i && i || i);
+ // will be expanded to:
+ // i && i || i && i || i
+ // ^ arg positon 2 (i)
+ //^ arg position 0 (&&)
+ // ^ arg position 3 (i)
+ // ^ arg position 1 (||) should not be checked because `op ||` and nothing from same arg position
+ //
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing updated this revision to Diff 151599.
https://reviews.llvm.org/D47687
Files:
lib/Sema/SemaExpr.cpp
test/Sema/parentheses.c
Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -14,6 +14,15 @@
if ((i = 4)) {}
}
+// testing macros
+// nesting macros testing
+#define NESTING_VOID(cond) ( (void)(cond) )
+#define NESTING_VOID_WRAPPER(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
+// non-nesting macros
+#define NON_NESTING_VOID_0(cond) ( (void)(cond) )
+#define NON_NESTING_VOID_1(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
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 +105,33 @@
(void)(i && i || 0); // no warning.
(void)(0 || i && i); // no warning.
+
+ //===--
+ // Logical operator in macros
+ //===--
+
+ // actionable expression tests
+ NON_NESTING_VOID_0(i && i || i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around
the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_0((i && i) || i); // no warning.
+
+ NON_NESTING_VOID_1(&&, ||, i, i && i || i, i); // expected-warning {{'&&'
within '||'}} \
+ // expected-note {{place
parentheses around the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_1(&&, ||, i, (i && i) || i, i); // no warning.
+
+ NESTING_VOID_WRAPPER(&&, ||, i, i && i || i, i); // expected-warning {{'&&'
within '||'}} \
+ // expected-note {{place
parentheses around the '&&' expression to silence this warning}}
+ NESTING_VOID_WRAPPER(&&, ||, i, (i && i) || i, i); // no warning.
+
+ // FIXME: strange things, but I think this could be tolerated
+ NON_NESTING_VOID_1(&&, ||, i && i || i, i, i); // no warning. but expected.
because op and LHS are not from same arg position
+ NON_NESTING_VOID_1(&&, ||, i, i, i && i || i); // no warning. but expected.
because op and RHS are not from same arg position
+ NESTING_VOID_WRAPPER(&&, ||, i && i || i, i, i); // no warning. but
expected. because op and LHS are not from same arg position
+ NESTING_VOID_WRAPPER(&&, ||, i, i, i && i || i); // no warning. but
expected. because op and RHS are not from same arg position
+
+ // un-actionable expression tests
+ NESTING_VOID_WRAPPER(&&, ||, i, i, i); // no warning.
+ NON_NESTING_VOID_1(&&, ||, i, i, i); // no warning.
}
_Bool someConditionFunc();
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12310,9 +12310,26 @@
// 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);
+ if (Opc == BO_LOr) {
+if (!OpLoc.isMacroID()) {
+ // Operator is not in macros
+ 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,
+ ) &&
+ Self.getSourceManager().isMacroArgExpansion(LHSExpr->getExprLoc(),
+ ) &&
+ Self.getSourceManager().isMacroArgExpansion(RHSExpr->getExprLoc(),
+ )) {
+if (OpExpansionLoc == LHSExpansionLoc && OpExpansionLoc ==
RHSExpansionLoc) {
+ DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
+ DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+}
+ }
+}
}
if ((Opc == BO_Shl &&
LHSExpr->getType()->isIntegralType(Self.getASTContext()))
Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -14,6 +14,15 @@
if ((i = 4)) {}
}
+// testing macros
+// nesting macros testing
+#define NESTING_VOID(cond) ( (void)(cond) )
+#define NESTING_VOID_WRAPPER(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
+// non-nesting macros
+#define NON_NESTING_VOID_0(cond) ( (void)(cond) )
+#define NON_NESTING_VOID_1(op0, op1, x, y,
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
dexonsmith added reviewers: arphaman, ahatanak.
dexonsmith added subscribers: arphaman, ahatanak.
dexonsmith added a comment.
In https://reviews.llvm.org/D47687#1133222, @Higuoxing wrote:
> 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 (||)
SGTM!
> I think this could cover bunch of cases. But I think my code is not so
> beautiful... So, is there any suggestion?
I made a couple of comments on the tests, but I'd appreciate someone else
reviewing the code. @arphaman? @ahatanak?
Comment at: test/Sema/logical-op-parentheses-in-macros.c:37
+
+void logical_op_from_macro_arguments2(unsigned i) {
+ macro_op_parentheses_check_ops_args(||, &&, i, i, i); // no
warning.
Please also check that there's no warning with nested macros:
```
#define VOID(cond) (void)(cond)
#define BUILD_VOID(op1, op2, x, y, z) VOID(x op1 y op2 z)
void foo(unsigned i) { BUILD_VOID(&&, ||, i, i, i); }
```
Comment at: test/Sema/logical-op-parentheses-in-macros.c:52
+}
\ No newline at end of file
Phabricator seems to be reporting that there's a missing newline at the end of
the file.
https://reviews.llvm.org/D47687
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing added a comment. Ping https://reviews.llvm.org/D47687 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
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 , 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
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing updated this revision to Diff 151290.
Higuoxing added a comment.
Ping, well, I think I understand `@dexonsmith`'s idea.
`Actionable` macro argument is just like something like this
#define foo(x) ( (void)(x) )
when we using it by
foo(a && b || c);
we could add parentheses for it by
foo((a && b) || c);
However, the following example is `not actionable`
#define foo(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
when we using it by
foo(&&, ||, x, y, z);
because, we also probably use it by
foo(+, *, x, y, z)
So, we cannot add parentheses for it carelessly...
My opinion is that, to judge if an `expr` is actionable is to test if the op
and both LHS and RHS are from same context or same argument position from
macro... But I cannot find such API for indicating a `expression` expanded
position exactly. There are API like: `isMacroArgExpansion` and
`isMacroBodyExpansion` which is determined by row string positions...
So, what I can do now for this is that using API `isMacroArgExpansion()` to let
it check the parentheses defined or expanded totally inside a macro like
#define foo(x, y, z) ( x && y || z )
The shortage is that we cannot check parentheses for this case:
foo(x && y || z);
because the operator is expanded from macro arguments.
Thanks
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,49 @@
+// 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); // no 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); // no warning.
+ macro_op_parentheses_check(i || "I love LLVM" && i || i); // no 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); // logical_op_parentheses_check-warning {{'&&' within '||'}} \
+// logical_op_parentheses_check-note {{place parentheses around the '&&' expression to silence this 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 , 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))
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing added a comment. In https://reviews.llvm.org/D47687#1128757, @dexonsmith wrote: > > Yes, I think understand the patch; but I think it's the wrong direction. I > think we should just make the existing `-Wlogical-op-parentheses` smart > enough to show actionable warnings in macros (but suppress the ones that are > not actionable, like the internals of `foo(&&, ||, ...)`, rather than adding > `-Wlogical-op-parentheses-in-macros`, which sounds like it would be > permanently off-by-default. In https://reviews.llvm.org/D47687#1128757, @dexonsmith wrote: > In https://reviews.llvm.org/D47687#1127120, @Higuoxing wrote: > > > In https://reviews.llvm.org/D47687#1126607, @dexonsmith wrote: > > > > > In https://reviews.llvm.org/D47687#1126074, @lebedev.ri wrote: > > > > > > > In https://reviews.llvm.org/D47687#1126032, @Higuoxing wrote: > > > > > > > > > In https://reviews.llvm.org/D47687#1125926, @rnk wrote: > > > > > > > > > > > @dexonsmith is there someone from Apple who can comment on > > > > > > rdar://8678458 and the merits of disabling this warning in macros? > > > > > > I strongly suspect the original report was dealing with code like > > > > > > `assert(x || y && "str");`, if so we can go forward with this. > > > > > > > > > > > > @chandlerc I know you've hit this behavior difference vs. GCC > > > > > > before. Any thoughts on the proposed change? > > > > > > > > > > > > > > > > > > > > > > > > > In https://reviews.llvm.org/D47687#1125964, @dexonsmith wrote: > > > > > > > > > > > In https://reviews.llvm.org/D47687#1125926, @rnk wrote: > > > > > > > > > > > > > @dexonsmith is there someone from Apple who can comment on > > > > > > > rdar://8678458 and the merits of disabling this warning in > > > > > > > macros? I strongly suspect the original report was dealing with > > > > > > > code like `assert(x || y && "str");`, if so we can go forward > > > > > > > with this. > > > > > > > > > > > > > > > > > > There were two commits with this radar: r119537 and r119540. The > > > > > > main motivation was a deeply nested macro that when "inlined" > > > > > > included the moral equivalent of `#define DOUBLE_OP(OP1, OP2, X, Y, > > > > > > Z) (X OP1 Y OP2 Z)`. There was terrible note spew when the warning > > > > > > fired in this case, and the use case for the macro made the warning > > > > > > un-actionable. We decided to suppress the warning entirely: > > > > > > > > > > > > > As a general comment, the warning seems to be useless for macros; > > > > > > > I'll follow the example of warn_logical_instead_of_bitwise which > > > > > > > doesn't trigger for macros and I'll make the warning not warn for > > > > > > > macros. > > > > > > > > > > > > > > > Hi, Thank you, > > > > > > > > > > I noticed that `warn_logical_instead_of_bitwise ` will also skip > > > > > parentheses checking in macros... well, this patch seems not so > > > > > necessary... both ok for me ... depends on all of you :-) > > > > > > > > > > > > At worst, we can issue this warning in a new `-Wparentheses-in-macros` > > > > subgroup, which, if apple so insists, could be off-by-default. > > > > That would less worse than just completely silencing it for the entire > > > > world. > > > > > > > > > I’d be fine with strengthening the existing warning as long as there is > > > an actionable fix-it. I suspect if you suppress it when the relevant > > > expression is constructed from multiple macro arguments that will be good > > > enough. > > > > > > Thanks, currently, `[-Wparentheses | -Wlogical-op-parentheses]` will not > > emit warning for parentheses in macros. only if you add > > `[-Wlogical-op-parentheses-in-macros]` it will emit something like `'&&' > > within '||'` warning... > > > > However, `'&' within '|'` checking was disabled in macros as well... I > > don't know if this patch meet the needs... if this patch was ok, then, just > > as @lebedev.ri said, Maybe we could add a `[-Wparentheses-in-macros]` > > subgroup and add these warning into this new group, in the future... > > depends on users :-) any suggestion? > > > Yes, I think understand the patch; but I think it's the wrong direction. I > think we should just make the existing `-Wlogical-op-parentheses` smart > enough to show actionable warnings in macros (but suppress the ones that are > not actionable, like the internals of `foo(&&, ||, ...)`, rather than adding > `-Wlogical-op-parentheses-in-macros`, which sounds like it would be > permanently off-by-default. Thank you `@lebedev.ri` for reviewing my code and change the title and various things! Thank you `@dexonsmith` for helping me on backgrounds of this topic, I do agree with you, because those who did not participate in our discussion might not know this option `[-Wlogical-op-parentheses-in-macros]`, and make a special option for this warning seems a little bit strange ... I will have a try, thanks a lot! https://reviews.llvm.org/D47687
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
dexonsmith added a comment. In https://reviews.llvm.org/D47687#1127120, @Higuoxing wrote: > In https://reviews.llvm.org/D47687#1126607, @dexonsmith wrote: > > > In https://reviews.llvm.org/D47687#1126074, @lebedev.ri wrote: > > > > > In https://reviews.llvm.org/D47687#1126032, @Higuoxing wrote: > > > > > > > In https://reviews.llvm.org/D47687#1125926, @rnk wrote: > > > > > > > > > @dexonsmith is there someone from Apple who can comment on > > > > > rdar://8678458 and the merits of disabling this warning in macros? I > > > > > strongly suspect the original report was dealing with code like > > > > > `assert(x || y && "str");`, if so we can go forward with this. > > > > > > > > > > @chandlerc I know you've hit this behavior difference vs. GCC before. > > > > > Any thoughts on the proposed change? > > > > > > > > > > > > > > > > > > > > In https://reviews.llvm.org/D47687#1125964, @dexonsmith wrote: > > > > > > > > > In https://reviews.llvm.org/D47687#1125926, @rnk wrote: > > > > > > > > > > > @dexonsmith is there someone from Apple who can comment on > > > > > > rdar://8678458 and the merits of disabling this warning in macros? > > > > > > I strongly suspect the original report was dealing with code like > > > > > > `assert(x || y && "str");`, if so we can go forward with this. > > > > > > > > > > > > > > > There were two commits with this radar: r119537 and r119540. The > > > > > main motivation was a deeply nested macro that when "inlined" > > > > > included the moral equivalent of `#define DOUBLE_OP(OP1, OP2, X, Y, > > > > > Z) (X OP1 Y OP2 Z)`. There was terrible note spew when the warning > > > > > fired in this case, and the use case for the macro made the warning > > > > > un-actionable. We decided to suppress the warning entirely: > > > > > > > > > > > As a general comment, the warning seems to be useless for macros; > > > > > > I'll follow the example of warn_logical_instead_of_bitwise which > > > > > > doesn't trigger for macros and I'll make the warning not warn for > > > > > > macros. > > > > > > > > > > > > Hi, Thank you, > > > > > > > > I noticed that `warn_logical_instead_of_bitwise ` will also skip > > > > parentheses checking in macros... well, this patch seems not so > > > > necessary... both ok for me ... depends on all of you :-) > > > > > > > > > At worst, we can issue this warning in a new `-Wparentheses-in-macros` > > > subgroup, which, if apple so insists, could be off-by-default. > > > That would less worse than just completely silencing it for the entire > > > world. > > > > > > I’d be fine with strengthening the existing warning as long as there is an > > actionable fix-it. I suspect if you suppress it when the relevant > > expression is constructed from multiple macro arguments that will be good > > enough. > > > Thanks, currently, `[-Wparentheses | -Wlogical-op-parentheses]` will not emit > warning for parentheses in macros. only if you add > `[-Wlogical-op-parentheses-in-macros]` it will emit something like `'&&' > within '||'` warning... > > However, `'&' within '|'` checking was disabled in macros as well... I don't > know if this patch meet the needs... if this patch was ok, then, just as > @lebedev.ri said, Maybe we could add a `[-Wparentheses-in-macros]` subgroup > and add these warning into this new group, in the future... depends on users > :-) any suggestion? Yes, I think understand the patch; but I think it's the wrong direction. I think we should just make the existing `-Wlogical-op-parentheses` smart enough to show actionable warnings in macros (but suppress the ones that are not actionable, like the internals of `foo(&&, ||, ...)`, rather than adding `-Wlogical-op-parentheses-in-macros`, which sounds like it would be permanently off-by-default. https://reviews.llvm.org/D47687 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
