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,
+ &OpExpansionLoc) &&
+ Self.getSourceManager().isMacroArgExpansion(RHSExpr->getExprLoc(),
+ &LHSExpansionLoc))) {
----------------
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, &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);
> ```
>
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
