[PATCH] D60183: Fix typos in tests. NFC.
This revision was automatically updated to reflect the committed changes. Closed by commit rL357577: Fix typos in tests. NFC. (authored by Higuoxing, committed by ). Herald added a subscriber: delcypher. Changed prior to commit: https://reviews.llvm.org/D60183?vs=193439=193482#toc Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60183/new/ https://reviews.llvm.org/D60183 Files: cfe/trunk/test/Analysis/analyzer-list-configs.c compiler-rt/trunk/test/tsan/race_on_heap.cc Index: compiler-rt/trunk/test/tsan/race_on_heap.cc === --- compiler-rt/trunk/test/tsan/race_on_heap.cc +++ compiler-rt/trunk/test/tsan/race_on_heap.cc @@ -39,7 +39,7 @@ // CHECK: WARNING: ThreadSanitizer: data race // ... // CHECK: Location is heap block of size 99 at [[ADDR]] allocated by thread T1: -// CHCEK: #0 malloc +// CHECK: #0 malloc // CHECK: #{{1|2}} alloc // CHECK: #{{2|3}} AllocThread // ... Index: cfe/trunk/test/Analysis/analyzer-list-configs.c === --- cfe/trunk/test/Analysis/analyzer-list-configs.c +++ cfe/trunk/test/Analysis/analyzer-list-configs.c @@ -3,7 +3,7 @@ // // CHECK: USAGE: clang -cc1 [CLANG_OPTIONS] -analyzer-config // -// CHCEK: clang -cc1 [CLANG_OPTIONS] -analyzer-config OPTION1=VALUE, -analyzer-config OPTION2=VALUE, ... +// CHECK: clang -cc1 [CLANG_OPTIONS] -analyzer-config OPTION1=VALUE, -analyzer-config OPTION2=VALUE, ... // // CHECK: clang [CLANG_OPTIONS] -Xclang -analyzer-config -Xclang // Index: compiler-rt/trunk/test/tsan/race_on_heap.cc === --- compiler-rt/trunk/test/tsan/race_on_heap.cc +++ compiler-rt/trunk/test/tsan/race_on_heap.cc @@ -39,7 +39,7 @@ // CHECK: WARNING: ThreadSanitizer: data race // ... // CHECK: Location is heap block of size 99 at [[ADDR]] allocated by thread T1: -// CHCEK: #0 malloc +// CHECK: #0 malloc // CHECK: #{{1|2}} alloc // CHECK: #{{2|3}} AllocThread // ... Index: cfe/trunk/test/Analysis/analyzer-list-configs.c === --- cfe/trunk/test/Analysis/analyzer-list-configs.c +++ cfe/trunk/test/Analysis/analyzer-list-configs.c @@ -3,7 +3,7 @@ // // CHECK: USAGE: clang -cc1 [CLANG_OPTIONS] -analyzer-config // -// CHCEK: clang -cc1 [CLANG_OPTIONS] -analyzer-config OPTION1=VALUE, -analyzer-config OPTION2=VALUE, ... +// CHECK: clang -cc1 [CLANG_OPTIONS] -analyzer-config OPTION1=VALUE, -analyzer-config OPTION2=VALUE, ... // // CHECK: clang [CLANG_OPTIONS] -Xclang -analyzer-config -Xclang // ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60183: Fix typos in tests. NFC.
Higuoxing created this revision. Herald added subscribers: llvm-commits, Sanitizers, cfe-commits, kubamracek. Herald added projects: clang, Sanitizers, LLVM. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D60183 Files: clang/test/Analysis/analyzer-list-configs.c compiler-rt/test/tsan/race_on_heap.cc Index: compiler-rt/test/tsan/race_on_heap.cc === --- compiler-rt/test/tsan/race_on_heap.cc +++ compiler-rt/test/tsan/race_on_heap.cc @@ -39,7 +39,7 @@ // CHECK: WARNING: ThreadSanitizer: data race // ... // CHECK: Location is heap block of size 99 at [[ADDR]] allocated by thread T1: -// CHCEK: #0 malloc +// CHECK: #0 malloc // CHECK: #{{1|2}} alloc // CHECK: #{{2|3}} AllocThread // ... Index: clang/test/Analysis/analyzer-list-configs.c === --- clang/test/Analysis/analyzer-list-configs.c +++ clang/test/Analysis/analyzer-list-configs.c @@ -3,7 +3,7 @@ // // CHECK: USAGE: clang -cc1 [CLANG_OPTIONS] -analyzer-config // -// CHCEK: clang -cc1 [CLANG_OPTIONS] -analyzer-config OPTION1=VALUE, -analyzer-config OPTION2=VALUE, ... +// CHECK: clang -cc1 [CLANG_OPTIONS] -analyzer-config OPTION1=VALUE, -analyzer-config OPTION2=VALUE, ... // // CHECK: clang [CLANG_OPTIONS] -Xclang -analyzer-config -Xclang // Index: compiler-rt/test/tsan/race_on_heap.cc === --- compiler-rt/test/tsan/race_on_heap.cc +++ compiler-rt/test/tsan/race_on_heap.cc @@ -39,7 +39,7 @@ // CHECK: WARNING: ThreadSanitizer: data race // ... // CHECK: Location is heap block of size 99 at [[ADDR]] allocated by thread T1: -// CHCEK: #0 malloc +// CHECK: #0 malloc // CHECK: #{{1|2}} alloc // CHECK: #{{2|3}} AllocThread // ... Index: clang/test/Analysis/analyzer-list-configs.c === --- clang/test/Analysis/analyzer-list-configs.c +++ clang/test/Analysis/analyzer-list-configs.c @@ -3,7 +3,7 @@ // // CHECK: USAGE: clang -cc1 [CLANG_OPTIONS] -analyzer-config // -// CHCEK: clang -cc1 [CLANG_OPTIONS] -analyzer-config OPTION1=VALUE, -analyzer-config OPTION2=VALUE, ... +// CHECK: clang -cc1 [CLANG_OPTIONS] -analyzer-config OPTION1=VALUE, -analyzer-config OPTION2=VALUE, ... // // CHECK: clang [CLANG_OPTIONS] -Xclang -analyzer-config -Xclang // ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present
Higuoxing added inline comments. Comment at: clang/include/clang/Format/Format.h:247 + /// When used in conjuction with ``AllowShortIfIfStatementsOnASingleLine`` + /// then when ``true``, ``if (a) return;`` can be put on a single even when Small nit: ``` AllowShortIfIfStatementsOnASingleLine ^ ``` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59087/new/ https://reviews.llvm.org/D59087 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://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)
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)
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)
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)
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)
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)
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: fix: [Bug 18971] - Missing -Wparentheses warning
Higuoxing marked 3 inline comments as done. Higuoxing added a comment. 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? 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: fix: [Bug 18971] - Missing -Wparentheses warning
Higuoxing added a comment. This diff version pass the both `parentheses.c` and `logical-op-parentheses.c` 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: fix: [Bug 18971] - Missing -Wparentheses warning
Higuoxing updated this revision to Diff 150517. https://reviews.llvm.org/D47687 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td 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,28 @@ +// RUN: %clang_cc1 -fsyntax-only -verify=silence %s +// RUN: %clang_cc1 -Wparentheses -fsyntax-only -verify=silence %s +// RUN: %clang_cc1 -Wlogical-op-parentheses-in-macros -fsyntax-only \ +// RUN:-verify=logical-op-parentheses %s +// RUN: %clang_cc1 -Wlogical-op-parentheses -fsyntax-only \ +// RUN:-verify=silence %s + +#define macro_op_parentheses_check(x) ( \ + ( (void)(x) ) \ +) + +// silence-no-diagnostics +void logical_op_in_macros_test(unsigned i) { + macro_op_parentheses_check(i || i && i); // logical-op-parentheses-warning {{'&&' within '||'}} \ + // logical-op-parentheses-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-warning {{'&&' within '||'}} \ +// logical-op-parentheses-note {{place parentheses around the '&&' expression to silence this warning}} + + macro_op_parentheses_check(i || "I love LLVM" && i || i); // logical-op-parentheses-warning {{'&&' within '||'}} \ +// logical-op-parentheses-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. +} Index: lib/Sema/SemaExpr.cpp === --- lib/Sema/SemaExpr.cpp +++ lib/Sema/SemaExpr.cpp @@ -12172,8 +12172,16 @@ EmitDiagnosticForLogicalAndInLogicalOr(Sema , SourceLocation OpLoc, BinaryOperator *Bop) { assert(Bop->getOpcode() == BO_LAnd); - Self.Diag(Bop->getOperatorLoc(), diag::warn_logical_and_in_logical_or) - << Bop->getSourceRange() << OpLoc; + Self.Diag(Bop->getOperatorLoc(), !OpLoc.isMacroID() ? +// if this warning is in a normal context +diag::warn_logical_and_in_logical_or : +// else this warning is in a macro context +// currently, this will not warn in macros by default. +// add [-Wlogical-op-parentheses-in-macros] +// to enable this warning. +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,10 @@ def warn_logical_and_in_logical_or : Warning< "'&&' within '||'">, InGroup; +def warn_logical_and_in_logical_or_in_macros: + Warning, + InGroup, DefaultIgnore; + def warn_overloaded_shift_in_comparison :Warning< "overloaded operator %select{>>|<<}0 has higher precedence than "
[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning
Higuoxing marked 2 inline comments as done. Higuoxing added a comment. Sorry, I will check it and update the test case Besides, shall I add the macro-parentheses checking test cases to the original 'parentheses.c'? 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: fix: [Bug 18971] - Missing -Wparentheses warning
Higuoxing updated this revision to Diff 150507. Higuoxing marked 4 inline comments as done. Higuoxing added a comment. Hope this not disturb you too much : ) thanks https://reviews.llvm.org/D47687 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td 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,26 @@ +// RUN: %clang_cc1 -Wlogical-op-parentheses-in-macros -fsyntax-only \ +// RUN:-verify=logical-op-parentheses %s +// RUN: %clang_cc1 -fsyntax-only %s +// RUN: %clang_cc1 -Wparentheses -fsyntax-only %s + +#define macro_op_parentheses_check(x) ( \ + ( (void)(x) ) \ +) + +void logical_op_in_macros_test(unsigned i) { + + macro_op_parentheses_check(i || i && i); // logical-op-parentheses-warning {{'&&' within '||'}} \ + // logical-op-parentheses-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-warning {{'&&' within '||'}} \ +// logical-op-parentheses-note {{place parentheses around the '&&' expression to silence this warning}} + + macro_op_parentheses_check(i || "I love LLVM" && i || i); // logical-op-parentheses-warning {{'&&' within '||'}} \ +// logical-op-parentheses-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. +} Index: lib/Sema/SemaExpr.cpp === --- lib/Sema/SemaExpr.cpp +++ lib/Sema/SemaExpr.cpp @@ -12172,8 +12172,16 @@ EmitDiagnosticForLogicalAndInLogicalOr(Sema , SourceLocation OpLoc, BinaryOperator *Bop) { assert(Bop->getOpcode() == BO_LAnd); - Self.Diag(Bop->getOperatorLoc(), diag::warn_logical_and_in_logical_or) - << Bop->getSourceRange() << OpLoc; + Self.Diag(Bop->getOperatorLoc(), !OpLoc.isMacroID() ? +// if this warning is in a normal context +diag::warn_logical_and_in_logical_or : +// else this warning is in a macro context +// currently, this will not warn in macros by default. +// add [-Wlogical-op-parentheses-in-macros] +// to enable this warning. +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,10 @@ def warn_logical_and_in_logical_or : Warning< "'&&' within '||'">, InGroup; +def warn_logical_and_in_logical_or_in_macros: + Warning, + InGroup, DefaultIgnore; + def warn_overloaded_shift_in_comparison :Warning< "overloaded operator %select{>>|<<}0 has higher precedence than " "comparison operator">, Index:
[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning
Higuoxing updated this revision to Diff 150495. Higuoxing added a comment. Update without conflict with test case `parentheses.c`... I add a separate option [-Wlogical-op-parentheses-in-macros] and will be silence by default thanks https://reviews.llvm.org/D47687 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td 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,23 @@ +// RUN: %clang_cc1 -Wlogical-op-parentheses-in-macros -fsyntax-only -verify %s + +#define macro_op_parentheses_check(x) ( \ + ( (void)(x) ) \ +) + +void logical_op_in_macros_test(unsigned i) { + + macro_op_parentheses_check(i || i && i); // expected-warning {{'&&' within '||'}} \ +// expected-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); // expected-warning {{'&&' within '||'}} \ + // expected-note {{place parentheses around the '&&' expression to silence this warning}} + + macro_op_parentheses_check(i || "I love LLVM" && i || i); // expected-warning {{'&&' within '||'}} \ + // expected-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. +} Index: lib/Sema/SemaExpr.cpp === --- lib/Sema/SemaExpr.cpp +++ lib/Sema/SemaExpr.cpp @@ -12172,8 +12172,16 @@ EmitDiagnosticForLogicalAndInLogicalOr(Sema , SourceLocation OpLoc, BinaryOperator *Bop) { assert(Bop->getOpcode() == BO_LAnd); - Self.Diag(Bop->getOperatorLoc(), diag::warn_logical_and_in_logical_or) - << Bop->getSourceRange() << OpLoc; + Self.Diag(Bop->getOperatorLoc(), !OpLoc.isMacroID() ? +// if this warning is in a normal context +diag::warn_logical_and_in_logical_or : +// else this warning is in a macro context +// currently, this will not warn in macros by default. +// add [-Wlogical-op-parentheses-in-macros] +// to enable this warning. +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; +def warn_logical_and_in_logical_or_in_macros: Warning< + warn_logical_and_in_logical_or.Text>, InGroup, DefaultIgnore; + def warn_overloaded_shift_in_comparison :Warning< "overloaded operator %select{>>|<<}0 has higher precedence than " "comparison operator">, Index: include/clang/Basic/DiagnosticGroups.td === ---
[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning
Higuoxing added a comment. Thanks, let me have a try 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: fix: [Bug 18971] - Missing -Wparentheses warning
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 , 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; +def warn_logical_and_in_logical_or_in_macros: Warning< + "'&&' within
[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning
Higuoxing added a comment. In https://reviews.llvm.org/D47687#1126074, @lebedev.ri wrote: > 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. Yes, that's exactly what I am thinking just now! I think I could make another new patch for this `warning` ... and then we could determine on which one to take 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: fix: [Bug 18971] - Missing -Wparentheses warning
Higuoxing added a comment. 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 :-) 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: fix: [Bug 18971] - Missing -Wparentheses warning
Higuoxing updated this revision to Diff 150253. Higuoxing added a reviewer: echristo. Higuoxing added a comment. update with comments & remove some useless blank lines. 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,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 @@ -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,8 +12311,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: 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. }