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