[clang] d1e3235 - [libTooling] Change Tranformer's consumer to take multiple changes
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 15 08:34:52 PST 2022
Author: Eric Li
Date: 2022-02-15T16:34:36Z
New Revision: d1e3235f604d65a62d25842305f54e43bd36681f
URL: https://github.com/llvm/llvm-project/commit/d1e3235f604d65a62d25842305f54e43bd36681f
DIFF: https://github.com/llvm/llvm-project/commit/d1e3235f604d65a62d25842305f54e43bd36681f.diff
LOG: [libTooling] Change Tranformer's consumer to take multiple changes
Previously, Transformer would invoke the consumer once per file modified per
match, in addition to any errors encountered. The consumer is not aware of which
AtomicChanges come from any particular match. It is unclear which sets of edits
may be related or whether an error invalidates any previously emitted changes.
Modify the signature of the consumer to accept a set of changes. This keeps
related changes (i.e. all edits from a single match) together, and clarifies
that errors don't produce partial changes.
Reviewed By: ymandel
Differential Revision: https://reviews.llvm.org/D119745
Added:
Modified:
clang/include/clang/Tooling/Transformer/Transformer.h
clang/lib/Tooling/Transformer/Transformer.cpp
clang/unittests/Tooling/TransformerTest.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Tooling/Transformer/Transformer.h b/clang/include/clang/Tooling/Transformer/Transformer.h
index 31feacba5e286..3e0a026a7f2aa 100644
--- a/clang/include/clang/Tooling/Transformer/Transformer.h
+++ b/clang/include/clang/Tooling/Transformer/Transformer.h
@@ -22,16 +22,25 @@ namespace tooling {
/// defined by the arguments of the constructor.
class Transformer : public ast_matchers::MatchFinder::MatchCallback {
public:
- using ChangeConsumer =
- std::function<void(Expected<clang::tooling::AtomicChange> Change)>;
-
- /// \param Consumer Receives each rewrite or error. Will not necessarily be
- /// called for each match; for example, if the rewrite is not applicable
- /// because of macros, but doesn't fail. Note that clients are responsible
+ /// Provides the set of changes to the consumer. The callback is free to move
+ /// or destructively consume the changes as needed.
+ ///
+ /// We use \c MutableArrayRef as an abstraction to provide decoupling, and we
+ /// expect the majority of consumers to copy or move the individual values
+ /// into a separate data structure.
+ using ChangeSetConsumer = std::function<void(
+ Expected<llvm::MutableArrayRef<AtomicChange>> Changes)>;
+
+ /// \param Consumer Receives all rewrites for a single match, or an error.
+ /// Will not necessarily be called for each match; for example, if the rule
+ /// generates no edits but does not fail. Note that clients are responsible
/// for handling the case that independent \c AtomicChanges conflict with each
/// other.
- Transformer(transformer::RewriteRule Rule, ChangeConsumer Consumer)
- : Rule(std::move(Rule)), Consumer(std::move(Consumer)) {}
+ explicit Transformer(transformer::RewriteRule Rule,
+ ChangeSetConsumer Consumer)
+ : Rule(std::move(Rule)), Consumer(std::move(Consumer)) {
+ assert(this->Consumer && "Consumer is empty");
+ }
/// N.B. Passes `this` pointer to `MatchFinder`. So, this object should not
/// be moved after this call.
@@ -43,8 +52,9 @@ class Transformer : public ast_matchers::MatchFinder::MatchCallback {
private:
transformer::RewriteRule Rule;
- /// Receives each successful rewrites as an \c AtomicChange.
- ChangeConsumer Consumer;
+ /// Receives sets of successful rewrites as an
+ /// \c llvm::ArrayRef<AtomicChange>.
+ ChangeSetConsumer Consumer;
};
} // namespace tooling
} // namespace clang
diff --git a/clang/lib/Tooling/Transformer/Transformer.cpp b/clang/lib/Tooling/Transformer/Transformer.cpp
index 7a4d8b45f189e..1219afc551868 100644
--- a/clang/lib/Tooling/Transformer/Transformer.cpp
+++ b/clang/lib/Tooling/Transformer/Transformer.cpp
@@ -65,6 +65,9 @@ void Transformer::run(const MatchFinder::MatchResult &Result) {
}
}
+ llvm::SmallVector<AtomicChange, 1> Changes;
+ Changes.reserve(ChangesByFileID.size());
for (auto &IDChangePair : ChangesByFileID)
- Consumer(std::move(IDChangePair.second));
+ Changes.push_back(std::move(IDChangePair.second));
+ Consumer(llvm::MutableArrayRef<AtomicChange>(Changes));
}
diff --git a/clang/unittests/Tooling/TransformerTest.cpp b/clang/unittests/Tooling/TransformerTest.cpp
index 9ef1c08a7cfa3..4ab39843de935 100644
--- a/clang/unittests/Tooling/TransformerTest.cpp
+++ b/clang/unittests/Tooling/TransformerTest.cpp
@@ -26,6 +26,7 @@ using ::clang::transformer::applyFirst;
using ::clang::transformer::before;
using ::clang::transformer::cat;
using ::clang::transformer::changeTo;
+using ::clang::transformer::editList;
using ::clang::transformer::makeRule;
using ::clang::transformer::member;
using ::clang::transformer::name;
@@ -36,6 +37,8 @@ using ::clang::transformer::RewriteRule;
using ::clang::transformer::statement;
using ::testing::ElementsAre;
using ::testing::IsEmpty;
+using ::testing::ResultOf;
+using ::testing::UnorderedElementsAre;
constexpr char KHeaderContents[] = R"cc(
struct string {
@@ -120,10 +123,11 @@ class ClangRefactoringTestBase : public testing::Test {
return *ChangedCode;
}
- Transformer::ChangeConsumer consumer() {
- return [this](Expected<AtomicChange> C) {
+ Transformer::ChangeSetConsumer consumer() {
+ return [this](Expected<MutableArrayRef<AtomicChange>> C) {
if (C) {
- Changes.push_back(std::move(*C));
+ Changes.insert(Changes.end(), std::make_move_iterator(C->begin()),
+ std::make_move_iterator(C->end()));
} else {
// FIXME: stash this error rather then printing.
llvm::errs() << "Error generating changes: "
@@ -799,7 +803,6 @@ TEST_F(TransformerTest, MultiChange) {
}
TEST_F(TransformerTest, EditList) {
- using clang::transformer::editList;
std::string Input = R"cc(
void foo() {
if (10 > 1.0)
@@ -827,7 +830,6 @@ TEST_F(TransformerTest, EditList) {
}
TEST_F(TransformerTest, Flatten) {
- using clang::transformer::editList;
std::string Input = R"cc(
void foo() {
if (10 > 1.0)
@@ -1638,4 +1640,46 @@ TEST_F(TransformerTest, AddIncludeMultipleFiles) {
<< "Could not update code: " << llvm::toString(UpdatedCode.takeError());
EXPECT_EQ(format(*UpdatedCode), format(Header));
}
+
+// A single change set can span multiple files.
+TEST_F(TransformerTest, MultiFileEdit) {
+ // NB: The fixture is unused for this test, but kept for the test suite name.
+ std::string Header = R"cc(void Func(int id);)cc";
+ std::string Source = R"cc(#include "input.h"
+ void Caller() {
+ int id = 0;
+ Func(id);
+ })cc";
+ int ErrorCount = 0;
+ std::vector<AtomicChanges> ChangeSets;
+ clang::ast_matchers::MatchFinder MatchFinder;
+ Transformer T(
+ makeRule(callExpr(callee(functionDecl(hasName("Func"))),
+ forEachArgumentWithParam(expr().bind("arg"),
+ parmVarDecl().bind("param"))),
+ editList({changeTo(node("arg"), cat("ARG")),
+ changeTo(node("param"), cat("PARAM"))})),
+ [&](Expected<MutableArrayRef<AtomicChange>> Changes) {
+ if (Changes)
+ ChangeSets.push_back(AtomicChanges(Changes->begin(), Changes->end()));
+ else
+ ++ErrorCount;
+ });
+ T.registerMatchers(&MatchFinder);
+ auto Factory = newFrontendActionFactory(&MatchFinder);
+ EXPECT_TRUE(runToolOnCodeWithArgs(
+ Factory->create(), Source, std::vector<std::string>(), "input.cc",
+ "clang-tool", std::make_shared<PCHContainerOperations>(),
+ {{"input.h", Header}}));
+
+ EXPECT_EQ(ErrorCount, 0);
+ EXPECT_THAT(
+ ChangeSets,
+ UnorderedElementsAre(UnorderedElementsAre(
+ ResultOf([](const AtomicChange &C) { return C.getFilePath(); },
+ "input.cc"),
+ ResultOf([](const AtomicChange &C) { return C.getFilePath(); },
+ "./input.h"))));
+}
+
} // namespace
More information about the cfe-commits
mailing list