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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
