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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.


================
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)))
----------------
li.zhe.hua wrote:
> ymandel wrote:
> > Is the idea that the skipping is now built into `getValue` via `getStorageLocation`?
> I don't believe I fixed this. d002495 addressed this by having integral casts route to the sub-expression.
Great. Just to be sure I understand: you mean that the `IgnoreCasts()` calls were already redundant because of d002495?


================
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);
----------------
li.zhe.hua wrote:
> ymandel wrote:
> > I thought this is the default behavior?
> The default behavior of `StmtVisitor` is that `VisitFoo` will call `VisitFooBase` where `Foo` derives from `FooBase` (e.g. the default implementation of `VisitExprWithCleanups` calls `VisitFullExpr`). The base case is a `VisitStmt` implementation that does [approximately nothing](https://github.com/llvm/llvm-project/blob/c2a5a87500d92c7c2e76c595f1b0f439b98b0aff/clang/include/clang/AST/StmtVisitor.h#L170-L171), especially if `RetTy = void`.
> 
> So in this case, I'm changing this to automatically ignore `ParenExpr` and `ExprWithCleanups` and visit the sub-expression, which is not the default behavior.
> 
> This isn't used when we call the transfer function from CFG-provided statements, because the CFG doesn't emit these nodes. However, this is relevant when we manually call the transfer function, e.g. from [here](https://github.com/llvm/llvm-project/blob/c2a5a87500d92c7c2e76c595f1b0f439b98b0aff/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp#L108).
Interesting. Thanks for the explanation!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124965