[PATCH] D124965: [clang][dataflow] Centralize expression skipping logic

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel accepted this revision.
ymandel added a comment.
This revision is now accepted and ready to land.

Thanks!



================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:368
 void Environment::setStorageLocation(const Expr &E, StorageLocation &Loc) {
-  assert(!isa<ExprWithCleanups>(&E));
-  assert(ExprToLoc.find(&E) == ExprToLoc.end());
-  ExprToLoc[&E] = &Loc;
+  assert(ExprToLoc.find(&ignoreCFGOmittedNodes(E)) == ExprToLoc.end());
+  ExprToLoc[&ignoreCFGOmittedNodes(E)] = &Loc;
----------------
maybe bind to a temporary to avoid doing this twice (in debug code, that is)? arguably not that important to optimize debug mode but may also improve readability.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:38-41
+  if (auto *LHSValue =
+          dyn_cast_or_null<BoolValue>(Env.getValue(LHS, SkipPast::Reference)))
+    if (auto *RHSValue =
+            dyn_cast_or_null<BoolValue>(Env.getValue(RHS, SkipPast::Reference)))
----------------
Is the idea that the skipping is now built into `getValue` via `getStorageLocation`?


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:545-561
+  void VisitParenExpr(const ParenExpr *S) {
+    // The CFG does not contain `ParenExpr` as top-level statements in basic
+    // blocks, however manual traversal to sub-expressions may encounter them.
+    // Redirect to the sub-expression.
+    auto *SubExpr = S->getSubExpr();
+    assert(SubExpr != nullptr);
+    Visit(SubExpr);
----------------
I thought this is the default behavior?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124965