[PATCH] D84409: [libTooling] Add an `EditGenerator` that applies a rule throughout a bound node.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 accepted this revision.
gribozavr2 added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:337
+// refer to nodes bound by the calling rule. `Rule` is not applied to the node
+// itself.
+EditGenerator rewriteDescendants(std::string NodeId, RewriteRule Rule);
----------------
Please use three slashes for doc comments.


================
Comment at: clang/lib/Tooling/Transformer/RewriteRule.cpp:194
+    for (auto &Matcher : transformer::detail::buildMatchers(Rule))
+      MF->addMatcher(forEachDescendantDynamically<T>(std::move(Nodes), Matcher),
+                     this);
----------------
Wouldn't this line move the same value multiple times?


================
Comment at: clang/lib/Tooling/Transformer/RewriteRule.cpp:208
+    }
+    // FIXME: some combinators legitimately return no changes.
+    if (Transformations->empty()) {
----------------
The fix seems trivial -- removing the if statement. Why not do that?


================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:422
+  std::string Input =
+      "int f(int x) { int y = x; { int z = x * x; } return x; }";
+  std::string Expected =
----------------
These tests do not demonstrate that the rewrite is only applied to the descendants of the specified AST node -- would be good to test that.


================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:1135
+
+TEST_F(TransformerTest, RewriteDescendantsUnboundNode) {
+  std::string Input =
----------------
Any particular reason to add these tests at the end of the file instead of grouped together with the rest of the tests for `rewriteDescendants` above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84409