[PATCH] D66397: [Diagnostics] Improve -Wxor-used-as-pow

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: lib/Sema/SemaExpr.cpp:54-55
+                                    const SourceLocation Loc,
+                                    const Expr *SubLHS = nullptr,
+                                    const Expr *SubRHS = nullptr);
+
----------------
I'd appreciate some comments on what these arguments should be when non-null.


================
Comment at: lib/Sema/SemaExpr.cpp:11031-11032
   // Do not diagnose macros.
-  if (Loc.isMacroID())
+  if (Loc.isMacroID() || XorLHS.get()->getBeginLoc().isMacroID() ||
+      XorRHS.get()->getBeginLoc().isMacroID())
     return;
----------------
I would appreciate it if this patch didn't also change the behavior for macros. That seems like a larger discussion that can happen in a separate patch.


================
Comment at: lib/Sema/SemaExpr.cpp:11052
+      Negative = (Opc == UO_Minus);
+      ExplicitPlus = (Opc == UO_Plus);
     } else {
----------------
`!Negative` (we already verified above that it's either the + or - operator, so if it's not one, it's the other.)?


================
Comment at: lib/Sema/SemaExpr.cpp:11067
+
+  // Do not diagnose 2 ^ 64, but allow special case (2 ^ 64) - 1.
+  if (SubLHS && SubRHS && (LeftSideValue != 2 || RightSideValue != 64))
----------------
This comment explains what the code does, but not why it does it. Given that we're adding special cases, I think more comments here explaining why this is valuable would be appreciated.


================
Comment at: lib/Sema/SemaExpr.cpp:11073
       LHSInt->getBeginLoc(), S.getLocForEndOfToken(RHSInt->getLocation()));
-  llvm::StringRef ExprStr =
+  std::string ExprStr =
       Lexer::getSourceText(ExprRange, S.getSourceManager(), S.getLangOpts());
----------------
Why is this type changing?


================
Comment at: lib/Sema/SemaExpr.cpp:11078
       CharSourceRange::getCharRange(Loc, S.getLocForEndOfToken(Loc));
-  llvm::StringRef XorStr =
+  std::string XorStr =
       Lexer::getSourceText(XorRange, S.getSourceManager(), S.getLangOpts());
----------------
Same question here.


================
Comment at: lib/Sema/SemaExpr.cpp:11130
+                              ? "ULLONG_MAX"
+                              : "-1LL";
+          ExprRange = CharSourceRange::getCharRange(
----------------
The two branches are not equivalent. What about `~0ULL` instead?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66397/new/

https://reviews.llvm.org/D66397