[PATCH] D131616: [clang][dataflow] Generalise match switch utility to other AST types and add a `CFGMatchSwitch` which currently handles `CFGStmt` and `CFGInitializer`.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/CFGMatchSwitch.h:24-25
+#include "clang/AST/Stmt.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/CFG.h"
----------------
These seem unnecessary.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/CFGMatchSwitch.h:28
+#include "clang/Analysis/FlowSensitive/MatchSwitch.h"
+#include "llvm/ADT/StringRef.h"
+#include <functional>
----------------
This seems unnecessary.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/CFGMatchSwitch.h:30
+#include <functional>
+#include <string>
+#include <utility>
----------------
This seems unnecessary.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/CFGMatchSwitch.h:32
+#include <utility>
+#include <vector>
+
----------------
This seems unnecessary.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/CFGMatchSwitch.h:55
+                                        MSActionT<NodeT, State, Result> A) && {
+    std::move(std::move(StmtBuilder).template CaseOf<NodeT>(M, A));
+    return std::move(*this);
----------------
Is the outer move necessary? Also, aren't we supposed to assign the result back to `StmtBuilder`? Same comment for `InitBuilder` below.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:1
 //===---- MatchSwitch.h -----------------------------------------*- C++ -*-===//
 //
----------------
Let's add a FIXME to rename the file to ASTMatchSwitch.h and update the comment below.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:47-51
+template <typename T> using MSMatcherT = ast_matchers::internal::Matcher<T>;
+
+template <typename T, typename State, typename Result = void>
+using MSActionT = std::function<Result(
+    const T *, const ast_matchers::MatchFinder::MatchResult &, State &)>;
----------------
What do you think about calling these `MatchSwitchMatcher` and `MatchSwitchAction`?


================
Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:64
+/// callbacks, which together define a switch that can be applied to a node
+/// whose type can be derived from `BaseT`. This structure can simplify the
+/// definition of `transfer` functions that rely on pattern-matching.
----------------



================
Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:90
   ///
-  ///  `Node` should be a subclass of `Stmt`.
-  template <typename Node>
-  MatchSwitchBuilder &&
-  CaseOf(ast_matchers::internal::Matcher<Stmt> M,
-         std::function<Result(const Node *,
-                              const ast_matchers::MatchFinder::MatchResult &,
-                              State &)>
-             A) && {
+  ///  `NodeT` should be derivable from `BaseT`.
+  template <typename NodeT>
----------------
Let's add a static assert.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:90
   ///
-  ///  `Node` should be a subclass of `Stmt`.
-  template <typename Node>
-  MatchSwitchBuilder &&
-  CaseOf(ast_matchers::internal::Matcher<Stmt> M,
-         std::function<Result(const Node *,
-                              const ast_matchers::MatchFinder::MatchResult &,
-                              State &)>
-             A) && {
+  ///  `NodeT` should be derivable from `BaseT`.
+  template <typename NodeT>
----------------
sgatev wrote:
> Let's add a static assert.



================
Comment at: clang/unittests/Analysis/FlowSensitive/CFGMatchSwitchTest.cpp:12
+#include "clang/AST/Decl.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/Stmt.h"
----------------
This seems unnecessary.


================
Comment at: clang/unittests/Analysis/FlowSensitive/CFGMatchSwitchTest.cpp:14-15
+#include "clang/AST/Stmt.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/CFG.h"
----------------
These seem unnecessary.


================
Comment at: clang/unittests/Analysis/FlowSensitive/CFGMatchSwitchTest.cpp:20-24
+#include <cstdint>
+#include <memory>
+#include <ostream>
+#include <string>
+#include <utility>
----------------
These seem unnecessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131616