[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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