[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)

[Xing via Phabricator via cfe-commits]

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