[PATCH] D131614: [clang][dataflow] Extend transfer functions for other `CFGElement`s

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:103
+  /// code being analysed.
+  virtual void transferCFGElement(const CFGElement *Element, Lattice &L,
+                                  Environment &Env) {}
----------------
Any reason not to call this one `transfer` too?


================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:157
 
+// Holds data structures required for running dataflow analysis.
+struct AnalysisContext {
----------------
Let's start struct and member comments with `///`.


================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:184
 ///   All predecessors of `Block` except those with loop back edges must have
-///   already been transferred. States in `BlockStates` that are set to
+///   already been transferred. States in `TC.BlockStates` that are set to
 ///   `llvm::None` represent basic blocks that are not evaluated yet.
----------------



================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:312
+/// Transfers `State` by evaluating each element in the `Block` based on the
+/// `TC.Analysis` specified.
+///
----------------



================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:314
+///
+/// Built in transfer functions (if the option for `ApplyBuiltinTransfer` is set
+/// by the analysis) will be applied to the element before evaluation by the
----------------



================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:328-340
+      switch (Element.getKind()) {
+      case CFGElement::Statement: {
+        builtinTransfer(Element.castAs<CFGStmt>(), State, AC);
+        break;
+      }
+      case CFGElement::Initializer: {
+        builtinTransfer(Element.castAs<CFGInitializer>(), State);
----------------
The level of abstraction for built-in and user code is different in this function which makes it hard to follow. Let's move this in a `builtinTransfer(const CFGElement &, TypeErasedDataflowAnalysisState &)` function that dispatches to one of the other built-in transfer functions. This way `transferCFGBlock` won't mix implementation details with the high level algorithm.


================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:341-347
+    }
+    // User-provided analysis
+    AC.Analysis.transferTypeErased(&Element, State.Lattice, State.Env);
+    // Post processing
+    if (PostVisitCFG) {
+      PostVisitCFG(Element, State);
+    }
----------------



================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:164-170
+    std::function<void(AnalysisData)> VerifyResults, ArrayRef<std::string> Args,
+    const tooling::FileContentMappings &VirtualMappedFiles = {}) {
+
+  std::function<void(ASTContext & Context, const CFGElement &,
+                     const TypeErasedDataflowAnalysisState &)>
+      PostVisitCFG = nullptr;
+  if (PostVisitStmt) {
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131614