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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

This is looking much closer to what I think we had in mind, so I mostly have some cleanup suggestions.



================
Comment at: clang/lib/Analysis/CFG.cpp:979
 
-    const BinaryOperator *BitOp = dyn_cast<BinaryOperator>(BoolExpr);
+    const BinaryOperator *BitOp = dyn_cast<BinaryOperator>(BoolExpr->IgnoreParens());
     if (BitOp && (BitOp->getOpcode() == BO_And ||
----------------
You can drop the `IgnoreParens()` here -- `BoolExpr` is either `LHSExpr` or `RHSExpr`, and both of those were initialized from a call to `IgnoreParens()`.


================
Comment at: clang/lib/Analysis/CFG.cpp:1013-1015
+  // Helper function to get the sub expressions also for the unary operators. If
+  // unary are encountered will solve manually. This function can return value
+  // as 12 and also -12.
----------------



================
Comment at: clang/lib/Analysis/CFG.cpp:1017-1019
+    // The value to return;
+    Optional<llvm::APInt> Value = llvm::None;
+
----------------
Removing an unnecessary variable.


================
Comment at: clang/lib/Analysis/CFG.cpp:1020-1021
+
+    const IntegerLiteral *IntLiteral = nullptr;
+    const UnaryOperator *UnOp = dyn_cast<UnaryOperator>(E->IgnoreParens());
+
----------------
Coding style nit -- if the type is spelled out explicitly in the initialization, we tend to prefer using `auto` for the type so we don't have to repeat the type information twice.

Also removes a declaration that's not needed.


================
Comment at: clang/lib/Analysis/CFG.cpp:1027
+      // Literal.
+      const Expr *SubExpr = UnOp->getSubExpr();
+      IntLiteral = dyn_cast<IntegerLiteral>(SubExpr->IgnoreParens());
----------------
This ensures everything past here doesn't have to care about parens.


================
Comment at: clang/lib/Analysis/CFG.cpp:1028-1031
+      IntLiteral = dyn_cast<IntegerLiteral>(SubExpr->IgnoreParens());
+
+      if (IntLiteral) {
+
----------------



================
Comment at: clang/lib/Analysis/CFG.cpp:1032
+
+        Value = IntLiteral->getValue();
+        // Getting the Operator Code to perform them manually.
----------------



================
Comment at: clang/lib/Analysis/CFG.cpp:1033-1034
+        Value = IntLiteral->getValue();
+        // Getting the Operator Code to perform them manually.
+        const UnaryOperatorKind OpCode = UnOp->getOpcode();
+
----------------



================
Comment at: clang/lib/Analysis/CFG.cpp:1041-1045
+          return -*Value;
+        case UO_Not:
+          return ~*Value;
+        case UO_LNot:
+          return *Value = !*Value;
----------------



================
Comment at: clang/lib/Analysis/CFG.cpp:1047
+        default:
+          return Value;
+        }
----------------
If there's a unary expression operating on an integer literal... it has to be one of the listed operations above, otherwise I don't think you can get here. So might as well assert and bail out rather than return a value that we didn't properly perform the unary operation on.


================
Comment at: clang/lib/Analysis/CFG.cpp:1050-1058
+
+    } else {
+
+      IntLiteral = dyn_cast<IntegerLiteral>(E->IgnoreParens());
+      if (IntLiteral)
+        return Value = IntLiteral->getValue();
+    }
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130510