dexonsmith requested changes to this revision.
dexonsmith added inline comments.
This revision now requires changes to proceed.
================
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);
}
----------------
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?
================
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()) {
----------------
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.
================
Comment at: lib/Sema/SemaExpr.cpp:12315
+ if (!OpLoc.isMacroID()) {
+ // Operator is not in macros
+ DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
----------------
I don't think this comment is valuable. It's just repeating the code in the
condition.
================
Comment at: lib/Sema/SemaExpr.cpp:12319
+ } else {
+ // This Expression is expanded from macro
+ SourceLocation LHSExpansionLoc, OpExpansionLoc, RHSExpansionLoc;
----------------
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.
================
Comment at: lib/Sema/SemaExpr.cpp:12321
+ SourceLocation LHSExpansionLoc, OpExpansionLoc, RHSExpansionLoc;
+ if ((Self.getSourceManager().isMacroArgExpansion(OpLoc,
+ &OpExpansionLoc) &&
----------------
This will be cleaner if you create a local reference to
`Self.getSourceManager()` called `SM`.
================
Comment at: lib/Sema/SemaExpr.cpp:12325-12328
+ (Self.getSourceManager().isMacroArgExpansion(OpLoc,
+ &OpExpansionLoc) &&
+ Self.getSourceManager().isMacroArgExpansion(RHSExpr->getExprLoc(),
+ &LHSExpansionLoc))) {
----------------
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, &OpExpansion))
return;
SourceLocation ExprExpansion;
if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), &ExprExpansion) &&
OpExpansion == ExprExpansion)
DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
if (SM.isMacroArgExpansion(RHSExpr->getExprLoc(), &ExprExpansion) &&
OpExpansion == ExprExpansion)
DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
```
================
Comment at: test/Sema/parentheses.c:133
+ // expected-note {{place
parentheses around the '&&' expression to silence this warning}}
+ NESTING_VOID_WRAPPER(&&, ||, ((i && i) || i), i, i); // no waring.
+
----------------
typo: should be "no warning".
https://reviews.llvm.org/D47687
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
