[PATCH] D120900: [clang][dataflow] Add `MatchSwitch` utility library.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked an inline comment as done.
ymandel added a comment.

Thanks for the fast review!



================
Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:51
+template <typename State>
+using MatchSwitch = std::function<void(const Stmt &, ASTContext &, State &)>;
+
----------------
xazax.hun wrote:
> When we instantiate this with `TransferState` we have `ASTContext` both as an argument and as a member of `State`. Is this intentional?
Yes, but...
The `ASTContext` is needed for the match itself, but the `State` type is not guaranteed to be `TransferState`, so won't necessarily hold the context.

But, we could reorganize a bit. Either: 
a) pass MatchFinder::MatchResult, instead of BoundNodes. That would bundle the nodes, context and source manager, which seems like a good idea.
b) bake TransferState into MatchSwitch, and make the parameter genericy named rather than "lattice".

I'm inclined towards the first option, since it seems "right" to give full access to the `MatchResult`. WDYT?


================
Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:110
+        size_t Index = 0;
+        if (ID.consume_front("Tag") && !ID.getAsInteger(10, Index) &&
+            Index < Actions.size()) {
----------------
xazax.hun wrote:
> This does not need to be addressed here but it looks like it could be a useful feature to be able to tag nodes with integer identifiers.
Great point. I'm not sure that would work here, fwiw, because we prefix with "Tag" here to protect against interfering with any IDs in the matcher. I suppose we could "reserve" a portion of the ID space.

But, in general, integers would make more sense in other situations that deal w/ matchers. The problem is that, unless we encode them as strings, the implementation inside the matchers could be kind of messy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120900