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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

The implementation LG, thanks! Just a few NITs and comments about the public interface and the 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).
----------------
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`.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:232
+// `makeOrderedRule({makeRule(m1, action1), makeRule(m2, action2), ...});`
+RewriteRule makeOrderedRule(ArrayRef<RewriteRule> Rules);
+
----------------
ymandel wrote:
> ilya-biryukov wrote:
> > Any ideas for a better name? `pickFirst`  or `applyFirst`?
> Yes, those are both better. Went w/ `applyFirst`.  Also considered `applyFirstMatch` but not sure that buys much clarity
`applyFirst` looks good, thanks!


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:240
 
+/// The following three functions function are a low-level part of the API,
+/// provided to support interpretation of a \c RewriteRule in a tool, like \c
----------------
ymandel wrote:
> ilya-biryukov wrote:
> > Could they be made private? If not, why?
> I tried to clarify this. But, I'm also fine splitting these three into a separate header file or moving them to the bottom of this one.  They're only exposed at all because TransformerTidy lives in a different directory and namespace.  WDYT?
I see. Maybe additionally put them into `namespace detail`?
A good way to make sure that any use is a red herring. (Otherwise clients would need to read the code to realize it's an internal function.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:195
 
+/// Joins multiple rules into a single rule that applies the first rule in
+/// `Rules` whose pattern matches a given node.
----------------
NIT: Maybe shorten a bit? Something like

```
Applies the first rule whose pattern matches, other rules are ignored
```


The current version has a bit too many details, so it's hard to grasp on a first read.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:215
+//
+// Ordered rules allow us to specify each independently:
+// ```
----------------
NIT: s/Ordered rules allow us/applyFirst allows to... ?

With a new name for the function, "ordered" rules can be confusing


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:231
+// ```
+// More generally, anywhere you'd use anyOf(m1.bind("m1"), m2.bind("m2")) and
+// then dispatch on those tags in your code to decide what to do, we'll lift
----------------
Maybe start with this? It's a great analogy. Something like this at the start of the comment would be great:
```
`applyFirst` is similar to `anyOf` matcher with a replace action attached to each of its cases...
```


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:239
+  for (auto &Rule : Rules) {
+    R.Cases.append(Rule.Cases.begin(), Rule.Cases.end());
+  }
----------------
NIT: remove redundant `{}`


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:262
+
+// Finds the rule that was "selected" -- that is, whose matcher triggered the
+// `MatchResult`.
----------------
NIT:s/Finds the rule/Finds the case
Would help avoid potential confusion...


================
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));
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61335