ymandel added inline comments.
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:196
+/// Joins multiple rules into a single rule that applies the first rule in
+/// `Rules` whose pattern matches a given node. All of the rules must use the
+/// same kind of matcher (that is, share a base class in the AST hierarchy).
----------------
ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > > All of the rules must use the same kind of matcher
> > >
> > > Could you elaborate why we want this restriction?
> > > Why they can't be of different kinds?
> > >
> > > It feels ok to have transformations to completely unrelated nodes and
> > > still apply the first one.
> > >
> > Agreed. The problem is purely from the implementation perspective. Since
> > anyOf() enforces this restriction, I need to either
> > 1. pass it on to the user
> > 2. infer the base kind of each matcher and place them in the appropriate
> > bucket.
> >
> > I figured for a first pass, we'd go with 1. if you disagree, I can add the
> > logic to bucket them and thereby support arbitrary combinations. FWIW, I
> > think it's a desirable feature, so its not wasted work. its just a matter
> > of splitting up the work.
> Ah, ok, so this is the restriction of `anyOf`.
indeed -- and all the other variadoc operations.
================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:250
+ assert(!CommonKind.isNone() && "Cases must have compatible matchers.");
+ return DynTypedMatcher::constructVariadic(
+ DynTypedMatcher::VO_AnyOf, CommonKind, taggedMatchers("Tag",
Rule.Cases));
----------------
ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > I wonder if there a better way to construct an `anyOf` matcher that can
> > > tell which case matched...
> > > It looks to be the reason for a more complicated interface on the
> > > transformer side, but I'm not sure there's a way out of it.
> > >
> > > Any ideas?
> > I don't quite follow. Which interface complication are you referring to?
> > FWIW, the reason the code here doesn't just use the anyOf() matches is that
> > it doesn't take a vector -- it only has a variadic form.
> E.g. the restriction that the matchers should have a common kind seems to
> come from the fact that we need to later find out which case matched.
>
> It this a limitation of the AST matchers design? E.g. I can't match both a
> type and an expression and bind different subnodes in each submatcher, right?
> E.g. the restriction that the matchers should have a common kind seems to
> come from the fact that we need to later find out which case matched.
>
> It this a limitation of the AST matchers design? E.g. I can't match both a
> type and an expression and bind different subnodes in each submatcher, right?
Yes, as far as I can tell, but I don't think its connected to the binding --
every `DynTypedMatcher` needs to report what kind it supports. IIRC, this is
connected to the desire to specialize matchers to types to provide some static
checking for matchers. Since there is no universal/root AST type, there's no
root kind, and we're stuck w/ this restriction. If we were willing for the
`Matcher` class to diverge from the AST, we could add a "universal" node kind,
but that's another discussion...
In practice, this is mitigated in matchers by forcing the user to call a
different `addMatcher` for each kind. But, that won't work for us since we want
to (ultimately) support rules of different (base) kinds.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61335/new/
https://reviews.llvm.org/D61335
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
