[PATCH] D130510: Missing tautological compare warnings due to unary operators

[Richard Trieu via Phabricator via cfe-commits]

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