[PATCH] D133698: [clang][dataflow] SignAnalysis, edgeTransfer, branchTransfer

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:127
+  // Default implementation is a Noop.
+  virtual void branchTransfer(bool Branch, const Stmt *S, Lattice &L,
+                              Environment &Env) {}
----------------
Please use CRTP (a non-virtual function here), and you'll need SFINAE to detect the presence of the overload of branchTransfer in branchTransferTypeErased.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h:86
                                   Environment &) = 0;
+  virtual void branchTransferTypeErased(bool Branch, const Stmt *,
+                                        TypeErasedLattice &, Environment &) = 0;
----------------
Please add a doc comment. Please add a parameter name for the condition, it is not obvious what it is.

Could you think of a more descriptive name for `Branch`? For example, if the second parameter is `Cond`, WDYT about `CondValue`?




================
Comment at: clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h:86
                                   Environment &) = 0;
+  virtual void branchTransferTypeErased(bool Branch, const Stmt *,
+                                        TypeErasedLattice &, Environment &) = 0;
----------------
gribozavr2 wrote:
> Please add a doc comment. Please add a parameter name for the condition, it is not obvious what it is.
> 
> Could you think of a more descriptive name for `Branch`? For example, if the second parameter is `Cond`, WDYT about `CondValue`?
> 
> 
WDYT about calling it `transferBranch...` instead of `branchTransfer...`?


================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:78
 public:
-  TerminatorVisitor(const StmtToEnvMap &StmtToEnv, Environment &Env,
+  using RetTy = std::pair<const Expr *, bool>;
+  TerminatorVisitor(TypeErasedDataflowAnalysis &Analysis,
----------------
Please add a comment to RetTy.


================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:148
 
+    bool Assumption = true;
     // The condition must be inverted for the successor that encompasses the
----------------
`CondValue`?


================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:253
+        auto [Cond, Assumption] =
+            TerminatorVisitor(Analysis, StmtToEnv, PredState.Env,
+                              blockIndexInPredecessor(*Pred, Block),
----------------
It would be nice to still call `branchTransfer` even when `BuiltinTransferOpts == false`. Please leave a TODO if you don't want to implement that.


================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:586
+               HoldsSignLattice(UnorderedElementsAre(Pair(Var("a"), Top()))))));
+}
+
----------------
This file is an amazing example of how to use the framework, but could you add a more direct unit test with a simpler lattice and analysis that exercises the new callback?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133698