rtrieu added a comment. 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. 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
