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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 9 inline comments as done and an inline comment as not done.
ymandel added a comment.

Thanks for the review. PTAL.



================
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:
> > 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.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:224
+//
+// RewriteRule R = makeOrderedRule({makeRule(two_calls, two_calls_action)},
+//                                 {makeRule(left_call, left_call_action)},
----------------
ilya-biryukov wrote:
> Why would we need braces around each call? Aren't they a rewrite rule in themselves?
typo. thanks!


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:232
+// `makeOrderedRule({makeRule(m1, action1), makeRule(m2, action2), ...});`
+RewriteRule makeOrderedRule(ArrayRef<RewriteRule> Rules);
+
----------------
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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61335