aaron.ballman added a comment. In D130510#3729864 <https://reviews.llvm.org/D130510#3729864>, @rtrieu wrote:
> In D130510#3728719 <https://reviews.llvm.org/D130510#3728719>, @aaron.ballman > wrote: > >> In D130510#3727096 <https://reviews.llvm.org/D130510#3727096>, @rtrieu wrote: >> >>> This patch has been moving back and forth between >>> `IsIntegerLiteralConstantExpr` and `getIntegerLiteralSubexpressionValue`. >>> The first function is preexisting and the second one is a new function. >>> The final patch seems to settle on using just >>> `getIntegerLiteralSubexpressionValue`. Can you explain why the existing >>> function does not meet your needs? It wasn't clear from the update >>> messages why you went that way. >> >> The existing function returns whether the expression is an ICE (sort of), >> the replacement function calculates the actual value and returns an optional >> APInt so you can perform operations on it directly. That said, a future >> refactoring could probably remove `IsIntegerLiteralConstantExpr()` and >> replace it with `getIntegerLiteralSubexpressionValue()`, but the only caller >> of `IsIntegerLiteralConstantExpr()` doesn't actually need the value at that >> point. > > In that case, I suggest adding a comment to pointing to the other function. > I expected that some day, someone will notice that the calculation for > literals is slightly different between the two warnings and we should be > helpful to them. I have no other concerns. That's a good suggestion, I've added two suggested edits for the comment. Thanks for the extra set of eyes on this review @rtrieu! ================ Comment at: clang/lib/Analysis/CFG.cpp:74-75 /// Returns true on constant values based around a single IntegerLiteral. /// Allow for use of parentheses, integer casts, and negative signs. static bool IsIntegerLiteralConstantExpr(const Expr *E) { // Allow parentheses ---------------- ================ Comment at: clang/lib/Analysis/CFG.cpp:1015-1016 + // which are an IntegerLiteral or a UnaryOperator and returns the value with + // all operations performed on it. + Optional<llvm::APInt> getIntegerLiteralSubexpressionValue(const Expr *E) { + ---------------- Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130510/new/ https://reviews.llvm.org/D130510 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
