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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov 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).
----------------
> 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.



================
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)},
----------------
Why would we need braces around each call? Aren't they a rewrite rule in themselves?


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:232
+// `makeOrderedRule({makeRule(m1, action1), makeRule(m2, action2), ...});`
+RewriteRule makeOrderedRule(ArrayRef<RewriteRule> Rules);
+
----------------
Any ideas for a better name? `pickFirst`  or `applyFirst`?


================
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
----------------
Could they be made private? If not, why?


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:247
+
+/// Returns the \c Action of \c Rule that was selected in the match result.
+const RewriteRule::Case &
----------------
s/Action/Case? Or am I missing something?


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:182
+// Determines whether A is higher than B in the class hierarchy.
+static bool isHigher(ASTNodeKind A, ASTNodeKind B) {
+  static auto QualKind = ASTNodeKind::getFromNodeKind<QualType>();
----------------
NIT: maybe name it `isBaseOf`? When talking about class hierarchies, using 'base' and 'derived' is a common convention.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61335