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

Thu Aug 18 07:59:05 PDT 2022

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