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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits