diff --git a/clang/include/clang/Tooling/Transformer/Transformer.h b/clang/include/clang/Tooling/Transformer/Transformer.h --- a/clang/include/clang/Tooling/Transformer/Transformer.h +++ b/clang/include/clang/Tooling/Transformer/Transformer.h @@ -22,16 +22,44 @@ /// defined by the arguments of the constructor. class Transformer : public ast_matchers::MatchFinder::MatchCallback { public: - using ChangeConsumer = - std::function Change)>; + using ChangeConsumer = std::function Change)>; + /// 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> Changes)>; + + /// Deprecated. Use the constructor accepting \c ChangesConsumer. + /// /// \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 /// 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)) {} + : Transformer(std::move(Rule), + [Consumer = std::move(Consumer)]( + Expected> Changes) { + if (Changes) + for (auto &Change : *Changes) + Consumer(std::move(Change)); + else + Consumer(Changes.takeError()); + }) {} + + /// \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, 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 +71,9 @@ private: transformer::RewriteRule Rule; - /// Receives each successful rewrites as an \c AtomicChange. - ChangeConsumer Consumer; + /// Receives sets of successful rewrites as an + /// \c llvm::ArrayRef. + ChangeSetConsumer Consumer; }; } // namespace tooling } // namespace clang diff --git a/clang/lib/Tooling/Transformer/Transformer.cpp b/clang/lib/Tooling/Transformer/Transformer.cpp --- a/clang/lib/Tooling/Transformer/Transformer.cpp +++ b/clang/lib/Tooling/Transformer/Transformer.cpp @@ -65,6 +65,9 @@ } } + llvm::SmallVector Changes; + Changes.reserve(ChangesByFileID.size()); for (auto &IDChangePair : ChangesByFileID) - Consumer(std::move(IDChangePair.second)); + Changes.push_back(std::move(IDChangePair.second)); + Consumer(llvm::MutableArrayRef(Changes)); } diff --git a/clang/unittests/Tooling/TransformerTest.cpp b/clang/unittests/Tooling/TransformerTest.cpp --- a/clang/unittests/Tooling/TransformerTest.cpp +++ b/clang/unittests/Tooling/TransformerTest.cpp @@ -26,6 +26,7 @@ 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::statement; using ::testing::ElementsAre; using ::testing::IsEmpty; +using ::testing::ResultOf; +using ::testing::UnorderedElementsAre; constexpr char KHeaderContents[] = R"cc( struct string { @@ -120,10 +123,11 @@ return *ChangedCode; } - Transformer::ChangeConsumer consumer() { - return [this](Expected C) { + Transformer::ChangeSetConsumer consumer() { + return [this](Expected> 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, EditList) { - using clang::transformer::editList; std::string Input = R"cc( void foo() { if (10 > 1.0) @@ -827,7 +830,6 @@ } TEST_F(TransformerTest, Flatten) { - using clang::transformer::editList; std::string Input = R"cc( void foo() { if (10 > 1.0) @@ -1638,4 +1640,46 @@ << "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 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> 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(), "input.cc", + "clang-tool", std::make_shared(), + {{"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