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