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