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