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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thanks for working on this! I have some suggestions, but you'll also need to add a release note at some point.



================
Comment at: clang/lib/Analysis/CFG.cpp:970-980
+    if (LHSExpr->EvaluateAsInt(IntExprResult, *Context)) {
+       // Evaluating value.
+      BoolExpr = RHSExpr;
+    }
+    else if (RHSExpr->EvaluateAsInt(IntExprResult, *Context)) {
       BoolExpr = LHSExpr;
     }
----------------
Coding style fix.


================
Comment at: clang/lib/Analysis/CFG.cpp:985
+                  BitOp->getOpcode() == BO_Or  || 
+                  BitOp->getOpcode() == BO_Xor)) {
       const Expr *LHSExpr2 = BitOp->getLHS()->IgnoreParens();
----------------
Be sure to add test coverage for this change.


================
Comment at: clang/lib/Analysis/CFG.cpp:986-988
       const Expr *LHSExpr2 = BitOp->getLHS()->IgnoreParens();
       const Expr *RHSExpr2 = BitOp->getRHS()->IgnoreParens();
+      
----------------



================
Comment at: clang/lib/Analysis/CFG.cpp:989-996
+      // If integer literal in expression identified then will save value.
+      Expr::EvalResult IntExprResult2; 
 
-      const IntegerLiteral *IntLiteral2 = dyn_cast<IntegerLiteral>(LHSExpr2);
-
-      if (!IntLiteral2)
-        IntLiteral2 = dyn_cast<IntegerLiteral>(RHSExpr2);
-
-      if (!IntLiteral2)
+      
+      if (LHSExpr2->EvaluateAsInt(IntExprResult2, *Context));
+      else if ( RHSExpr2->EvaluateAsInt(IntExprResult2, *Context));
+      else
----------------
(Be sure to re-run clang format over the patch, in case I've messed up 80-col limits with my suggestions.)


================
Comment at: clang/lib/Analysis/CFG.cpp:992
 
-      const IntegerLiteral *IntLiteral2 = dyn_cast<IntegerLiteral>(LHSExpr2);
-
-      if (!IntLiteral2)
-        IntLiteral2 = dyn_cast<IntegerLiteral>(RHSExpr2);
-
-      if (!IntLiteral2)
+      
+      if (LHSExpr2->EvaluateAsInt(IntExprResult2, *Context));
----------------
It looks like you've added a spurious tab here.


================
Comment at: clang/lib/Analysis/CFG.cpp:998
 
-      llvm::APInt L1 = IntLiteral->getValue();
-      llvm::APInt L2 = IntLiteral2->getValue();
+      // Getting the values as integer from the evaluation expression to use for comparision.
+      llvm::APInt L1 = IntExprResult.Val.getInt(); 
----------------
Take this suggestion or leave it, as you want. I don't think the comment added much value because it's describing what the code does (which is reasonably obvious from reading the code given that the code is pretty simple) rather than describing why the code is necessary (which doesn't seem to be required in this case).


================
Comment at: clang/lib/Analysis/CFG.cpp:1008
                                                      B->getOpcode() != BO_EQ);
         TryResult(B->getOpcode() != BO_EQ);
       }
----------------
This looks like an existing bug and suggests we're missing test coverage -- we create a `TryResult` object but do nothing with it; I suspect we wanted to return this result.

Can you try to add test coverage that hits this code path to verify the current behavior is wrong, then change it to return the result to make sure the behavior is corrected?


================
Comment at: clang/lib/Analysis/CFG.cpp:1011-1014
+      llvm::APInt IntValue = IntExprResult.Val.getInt(); // Getting the value.
       if ((IntValue == 1) || (IntValue == 0)) {
         return TryResult();
       }
----------------



================
Comment at: clang/test/SemaCXX/warn-unreachable.cpp:400-401
   // TODO: Extend warning to the following code:
   if (x < -1)
     calledFun();
+  if (x == -1)   // expected-note {{silence}}
----------------
Why aren't we catching this one?


================
Comment at: clang/test/SemaCXX/warn-unreachable.cpp:409-410
+    calledFun(); // expected-warning {{will never be executed}}
   if (-1 > x)
     calledFun();
   else
----------------
Or this one?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130510/new/

https://reviews.llvm.org/D130510