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