[PATCH] D87031: [libTooling] Provide overloads of `rewriteDescendants` that operate directly on an AST node.
gribozavr2 added inline comments. Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:388 +llvm::Expected> +rewriteDescendants(const Decl , RewriteRule Rule, + const ast_matchers::MatchFinder::MatchResult ); ymandel wrote: > gribozavr2 wrote: > > I agree that these functions provide very useful functionality, however I > > feel uneasy about putting a function that returns an `EditGenerator` and a > > function that actually executes the refactoring into the same overload set. > > The first is a part of a DSL, the second is a regular function. Do you > > think this is a problem worth solving? > > > > IDK what exactly to suggest though. A namespace for DSL functions like we > > have in AST matchers? > That's a really good point. These new functions are definitely a part of an > (implicit) lower-level library that improves manipulation of the AST > directly. We don't have any appropriate library yet for this -- SourceCode > is in this spirit but even simpler than RewriteRule, which in fact depends on > it. > > For now, I'll put these in the `detail` which is (in some sense) the > collection of low level functions for which we need to solve this same > problem. WDYT? (I wasn't quite clear about the comparison with ast matchers, > since both the matchers DSL and the `match` functions (on which this is > loosely based) are in the same namespace). > I wasn't quite clear about the comparison with ast matchers, since both the > matchers DSL and the match functions (on which this is loosely based) are in > the same namespace Oh right -- I mistakenly thought that `clang::ast_matchers` only contains the DSL. I think your choice to put functions into `detail` looks good. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87031/new/ https://reviews.llvm.org/D87031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87031: [libTooling] Provide overloads of `rewriteDescendants` that operate directly on an AST node.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd4f390313129: [libTooling] Provide overloads of
`rewriteDescendants` that operate directly on… (authored by ymandel).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87031/new/
https://reviews.llvm.org/D87031
Files:
clang/include/clang/Tooling/Transformer/RewriteRule.h
clang/lib/Tooling/Transformer/RewriteRule.cpp
clang/unittests/Tooling/TransformerTest.cpp
Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -25,6 +25,7 @@
using ::testing::IsEmpty;
using transformer::cat;
using transformer::changeTo;
+using transformer::rewriteDescendants;
using transformer::RewriteRule;
constexpr char KHeaderContents[] = R"cc(
@@ -568,6 +569,88 @@
EXPECT_EQ(ErrorCount, 1);
}
+//
+// We include one test per typed overload. We don't test extensively since that
+// is already covered by the tests above.
+//
+
+TEST_F(TransformerTest, RewriteDescendantsTypedStmt) {
+ // Add an unrelated definition to the header that also has a variable named
+ // "x", to test that the rewrite is limited to the scope we intend.
+ appendToHeader(R"cc(int g(int x) { return x; })cc");
+ std::string Input =
+ "int f(int x) { int y = x; { int z = x * x; } return x; }";
+ std::string Expected =
+ "int f(int x) { int y = 3; { int z = 3 * 3; } return 3; }";
+ auto InlineX =
+ makeRule(declRefExpr(to(varDecl(hasName("x", changeTo(cat("3")));
+ testRule(makeRule(functionDecl(hasName("f"), hasBody(stmt().bind("body"))),
+[](const MatchFinder::MatchResult ) {
+ const auto *Node = R.Nodes.getNodeAs("body");
+ assert(Node != nullptr && "body must be bound");
+ return transformer::detail::rewriteDescendants(
+ *Node, InlineX, R);
+}),
+ Input, Expected);
+}
+
+TEST_F(TransformerTest, RewriteDescendantsTypedDecl) {
+ std::string Input =
+ "int f(int x) { int y = x; { int z = x * x; } return x; }";
+ std::string Expected =
+ "int f(int x) { int y = 3; { int z = 3 * 3; } return 3; }";
+ auto InlineX =
+ makeRule(declRefExpr(to(varDecl(hasName("x", changeTo(cat("3")));
+ testRule(makeRule(functionDecl(hasName("f")).bind("fun"),
+[](const MatchFinder::MatchResult ) {
+ const auto *Node = R.Nodes.getNodeAs("fun");
+ assert(Node != nullptr && "fun must be bound");
+ return transformer::detail::rewriteDescendants(
+ *Node, InlineX, R);
+}),
+ Input, Expected);
+}
+
+TEST_F(TransformerTest, RewriteDescendantsTypedTypeLoc) {
+ std::string Input = "int f(int *x) { return *x; }";
+ std::string Expected = "int f(char *x) { return *x; }";
+ auto IntToChar =
+ makeRule(typeLoc(loc(qualType(isInteger(), builtinType(.bind("loc"),
+ changeTo(cat("char")));
+ testRule(
+ makeRule(
+ functionDecl(
+ hasName("f"),
+ hasParameter(0, varDecl(hasTypeLoc(typeLoc().bind("parmType"),
+ [](const MatchFinder::MatchResult ) {
+const auto *Node = R.Nodes.getNodeAs("parmType");
+assert(Node != nullptr && "parmType must be bound");
+return transformer::detail::rewriteDescendants(*Node, IntToChar, R);
+ }),
+ Input, Expected);
+}
+
+TEST_F(TransformerTest, RewriteDescendantsTypedDynTyped) {
+ // Add an unrelated definition to the header that also has a variable named
+ // "x", to test that the rewrite is limited to the scope we intend.
+ appendToHeader(R"cc(int g(int x) { return x; })cc");
+ std::string Input =
+ "int f(int x) { int y = x; { int z = x * x; } return x; }";
+ std::string Expected =
+ "int f(int x) { int y = 3; { int z = 3 * 3; } return 3; }";
+ auto InlineX =
+ makeRule(declRefExpr(to(varDecl(hasName("x", changeTo(cat("3")));
+ testRule(
+ makeRule(functionDecl(hasName("f"), hasBody(stmt().bind("body"))),
+ [](const MatchFinder::MatchResult ) {
+ auto It = R.Nodes.getMap().find("body");
+ assert(It != R.Nodes.getMap().end() && "body must be bound");
+ return transformer::detail::rewriteDescendants(It->second,
+InlineX, R);
+ }),
+ Input, Expected);
+}
+
TEST_F(TransformerTest, InsertBeforeEdit) {
std::string Input = R"cc(
int f() {
Index: clang/lib/Tooling/Transformer/RewriteRule.cpp
===
--- clang/lib/Tooling/Transformer/RewriteRule.cpp
+++
[PATCH] D87031: [libTooling] Provide overloads of `rewriteDescendants` that operate directly on an AST node.
ymandel updated this revision to Diff 289623.
ymandel added a comment.
Herald added a subscriber: JDevlieghere.
fix diff base; make small clang-tidy suggested change
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87031/new/
https://reviews.llvm.org/D87031
Files:
clang/include/clang/Tooling/Transformer/RewriteRule.h
clang/lib/Tooling/Transformer/RewriteRule.cpp
clang/unittests/Tooling/TransformerTest.cpp
Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -25,6 +25,7 @@
using ::testing::IsEmpty;
using transformer::cat;
using transformer::changeTo;
+using transformer::rewriteDescendants;
using transformer::RewriteRule;
constexpr char KHeaderContents[] = R"cc(
@@ -568,6 +569,88 @@
EXPECT_EQ(ErrorCount, 1);
}
+//
+// We include one test per typed overload. We don't test extensively since that
+// is already covered by the tests above.
+//
+
+TEST_F(TransformerTest, RewriteDescendantsTypedStmt) {
+ // Add an unrelated definition to the header that also has a variable named
+ // "x", to test that the rewrite is limited to the scope we intend.
+ appendToHeader(R"cc(int g(int x) { return x; })cc");
+ std::string Input =
+ "int f(int x) { int y = x; { int z = x * x; } return x; }";
+ std::string Expected =
+ "int f(int x) { int y = 3; { int z = 3 * 3; } return 3; }";
+ auto InlineX =
+ makeRule(declRefExpr(to(varDecl(hasName("x", changeTo(cat("3")));
+ testRule(makeRule(functionDecl(hasName("f"), hasBody(stmt().bind("body"))),
+[](const MatchFinder::MatchResult ) {
+ const auto *Node = R.Nodes.getNodeAs("body");
+ assert(Node != nullptr && "body must be bound");
+ return transformer::detail::rewriteDescendants(
+ *Node, InlineX, R);
+}),
+ Input, Expected);
+}
+
+TEST_F(TransformerTest, RewriteDescendantsTypedDecl) {
+ std::string Input =
+ "int f(int x) { int y = x; { int z = x * x; } return x; }";
+ std::string Expected =
+ "int f(int x) { int y = 3; { int z = 3 * 3; } return 3; }";
+ auto InlineX =
+ makeRule(declRefExpr(to(varDecl(hasName("x", changeTo(cat("3")));
+ testRule(makeRule(functionDecl(hasName("f")).bind("fun"),
+[](const MatchFinder::MatchResult ) {
+ const auto *Node = R.Nodes.getNodeAs("fun");
+ assert(Node != nullptr && "fun must be bound");
+ return transformer::detail::rewriteDescendants(
+ *Node, InlineX, R);
+}),
+ Input, Expected);
+}
+
+TEST_F(TransformerTest, RewriteDescendantsTypedTypeLoc) {
+ std::string Input = "int f(int *x) { return *x; }";
+ std::string Expected = "int f(char *x) { return *x; }";
+ auto IntToChar =
+ makeRule(typeLoc(loc(qualType(isInteger(), builtinType(.bind("loc"),
+ changeTo(cat("char")));
+ testRule(
+ makeRule(
+ functionDecl(
+ hasName("f"),
+ hasParameter(0, varDecl(hasTypeLoc(typeLoc().bind("parmType"),
+ [](const MatchFinder::MatchResult ) {
+const auto *Node = R.Nodes.getNodeAs("parmType");
+assert(Node != nullptr && "parmType must be bound");
+return transformer::detail::rewriteDescendants(*Node, IntToChar, R);
+ }),
+ Input, Expected);
+}
+
+TEST_F(TransformerTest, RewriteDescendantsTypedDynTyped) {
+ // Add an unrelated definition to the header that also has a variable named
+ // "x", to test that the rewrite is limited to the scope we intend.
+ appendToHeader(R"cc(int g(int x) { return x; })cc");
+ std::string Input =
+ "int f(int x) { int y = x; { int z = x * x; } return x; }";
+ std::string Expected =
+ "int f(int x) { int y = 3; { int z = 3 * 3; } return 3; }";
+ auto InlineX =
+ makeRule(declRefExpr(to(varDecl(hasName("x", changeTo(cat("3")));
+ testRule(
+ makeRule(functionDecl(hasName("f"), hasBody(stmt().bind("body"))),
+ [](const MatchFinder::MatchResult ) {
+ auto It = R.Nodes.getMap().find("body");
+ assert(It != R.Nodes.getMap().end() && "body must be bound");
+ return transformer::detail::rewriteDescendants(It->second,
+InlineX, R);
+ }),
+ Input, Expected);
+}
+
TEST_F(TransformerTest, InsertBeforeEdit) {
std::string Input = R"cc(
int f() {
Index: clang/lib/Tooling/Transformer/RewriteRule.cpp
===
--- clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -242,7 +242,7 @@
[PATCH] D87031: [libTooling] Provide overloads of `rewriteDescendants` that operate directly on an AST node.
ymandel added a comment. Thanks for the review! Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:388 +llvm::Expected> +rewriteDescendants(const Decl , RewriteRule Rule, + const ast_matchers::MatchFinder::MatchResult ); gribozavr2 wrote: > I agree that these functions provide very useful functionality, however I > feel uneasy about putting a function that returns an `EditGenerator` and a > function that actually executes the refactoring into the same overload set. > The first is a part of a DSL, the second is a regular function. Do you think > this is a problem worth solving? > > IDK what exactly to suggest though. A namespace for DSL functions like we > have in AST matchers? That's a really good point. These new functions are definitely a part of an (implicit) lower-level library that improves manipulation of the AST directly. We don't have any appropriate library yet for this -- SourceCode is in this spirit but even simpler than RewriteRule, which in fact depends on it. For now, I'll put these in the `detail` which is (in some sense) the collection of low level functions for which we need to solve this same problem. WDYT? (I wasn't quite clear about the comparison with ast matchers, since both the matchers DSL and the `match` functions (on which this is loosely based) are in the same namespace). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87031/new/ https://reviews.llvm.org/D87031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87031: [libTooling] Provide overloads of `rewriteDescendants` that operate directly on an AST node.
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:388 +llvm::Expected> +rewriteDescendants(const Decl , RewriteRule Rule, + const ast_matchers::MatchFinder::MatchResult ); I agree that these functions provide very useful functionality, however I feel uneasy about putting a function that returns an `EditGenerator` and a function that actually executes the refactoring into the same overload set. The first is a part of a DSL, the second is a regular function. Do you think this is a problem worth solving? IDK what exactly to suggest though. A namespace for DSL functions like we have in AST matchers? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87031/new/ https://reviews.llvm.org/D87031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87031: [libTooling] Provide overloads of `rewriteDescendants` that operate directly on an AST node.
ymandel created this revision.
ymandel added a reviewer: gribozavr.
Herald added a project: clang.
ymandel requested review of this revision.
The new overloads apply directly to a node, like the
`clang::ast_matchers::match` functions, Rather than generating an
`EditGenerator` combinator.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D87031
Files:
clang/include/clang/Tooling/Transformer/RewriteRule.h
clang/lib/Tooling/Transformer/RewriteRule.cpp
clang/unittests/Tooling/TransformerTest.cpp
Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -568,6 +568,84 @@
EXPECT_EQ(ErrorCount, 1);
}
+//
+// We include one test per typed overload. We don't test extensively since that
+// is already covered by the tests above.
+//
+
+TEST_F(TransformerTest, RewriteDescendantsTypedStmt) {
+ // Add an unrelated definition to the header that also has a variable named
+ // "x", to test that the rewrite is limited to the scope we intend.
+ appendToHeader(R"cc(int g(int x) { return x; })cc");
+ std::string Input =
+ "int f(int x) { int y = x; { int z = x * x; } return x; }";
+ std::string Expected =
+ "int f(int x) { int y = 3; { int z = 3 * 3; } return 3; }";
+ auto InlineX =
+ makeRule(declRefExpr(to(varDecl(hasName("x", changeTo(cat("3")));
+ testRule(makeRule(functionDecl(hasName("f"), hasBody(stmt().bind("body"))),
+[](const MatchFinder::MatchResult ) {
+ const auto *Node = R.Nodes.getNodeAs("body");
+ assert(Node != nullptr && "body must be bound");
+ return rewriteDescendants(*Node, InlineX, R);
+}),
+ Input, Expected);
+}
+
+TEST_F(TransformerTest, RewriteDescendantsTypedDecl) {
+ std::string Input =
+ "int f(int x) { int y = x; { int z = x * x; } return x; }";
+ std::string Expected =
+ "int f(int x) { int y = 3; { int z = 3 * 3; } return 3; }";
+ auto InlineX =
+ makeRule(declRefExpr(to(varDecl(hasName("x", changeTo(cat("3")));
+ testRule(makeRule(functionDecl(hasName("f")).bind("fun"),
+[](const MatchFinder::MatchResult ) {
+ const auto *Node = R.Nodes.getNodeAs("fun");
+ assert(Node != nullptr && "fun must be bound");
+ return rewriteDescendants(*Node, InlineX, R);
+}),
+ Input, Expected);
+}
+
+TEST_F(TransformerTest, RewriteDescendantsTypedTypeLoc) {
+ std::string Input = "int f(int *x) { return *x; }";
+ std::string Expected = "int f(char *x) { return *x; }";
+ auto IntToChar =
+ makeRule(typeLoc(loc(qualType(isInteger(), builtinType(.bind("loc"),
+ changeTo(cat("char")));
+ testRule(
+ makeRule(functionDecl(hasName("f"),
+hasParameter(0, varDecl(hasTypeLoc(
+typeLoc().bind("parmType"),
+ [](const MatchFinder::MatchResult ) {
+ const auto *Node = R.Nodes.getNodeAs("parmType");
+ assert(Node != nullptr && "parmType must be bound");
+ return rewriteDescendants(*Node, IntToChar, R);
+ }),
+ Input, Expected);
+}
+
+TEST_F(TransformerTest, RewriteDescendantsTypedDynTyped) {
+ // Add an unrelated definition to the header that also has a variable named
+ // "x", to test that the rewrite is limited to the scope we intend.
+ appendToHeader(R"cc(int g(int x) { return x; })cc");
+ std::string Input =
+ "int f(int x) { int y = x; { int z = x * x; } return x; }";
+ std::string Expected =
+ "int f(int x) { int y = 3; { int z = 3 * 3; } return 3; }";
+ auto InlineX =
+ makeRule(declRefExpr(to(varDecl(hasName("x", changeTo(cat("3")));
+ testRule(makeRule(functionDecl(hasName("f"), hasBody(stmt().bind("body"))),
+[](const MatchFinder::MatchResult ) {
+ auto It = R.Nodes.getMap().find("body");
+ assert(It != R.Nodes.getMap().end() &&
+ "body must be bound");
+ return rewriteDescendants(It->second, InlineX, R);
+}),
+ Input, Expected);
+}
+
TEST_F(TransformerTest, InsertBeforeEdit) {
std::string Input = R"cc(
int f() {
Index: clang/lib/Tooling/Transformer/RewriteRule.cpp
===
--- clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -242,7 +242,7 @@
} // namespace
template
-static llvm::Expected>
+llvm::Expected>
rewriteDescendantsImpl(const T , RewriteRule Rule,
const MatchResult ) {
ApplyRuleCallback Callback(std::move(Rule));
@@ -252,10 +252,42 @@
return
