[PATCH] D119745: [libTooling] Change Tranformer's consumer to take multiple changes

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd1e3235f604d: [libTooling] Change Tranformers consumer 
to take multiple changes (authored by li.zhe.hua, committed by ymandel).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119745

Files:
  clang/include/clang/Tooling/Transformer/Transformer.h
  clang/lib/Tooling/Transformer/Transformer.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ 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();
+  auto Factory = newFrontendActionFactory();
+  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 ) { return C.getFilePath(); },
+   "input.cc"),
+  ResultOf([](const AtomicChange ) { return C.getFilePath(); },
+   "./input.h";
+}
+
 } // namespace
Index: clang/lib/Tooling/Transformer/Transformer.cpp
===
--- clang/lib/Tooling/Transformer/Transformer.cpp
+++ clang/lib/Tooling/Transformer/Transformer.cpp
@@ -65,6 +65,9 @@
 }
   }
 
+  llvm::SmallVector Changes;
+  Changes.reserve(ChangesByFileID.size());
   for (auto  : ChangesByFileID)
-Consumer(std::move(IDChangePair.second));
+Changes.push_back(std::move(IDChangePair.second));
+  Consumer(llvm::MutableArrayRef(Changes));
 }
Index: clang/include/clang/Tooling/Transformer/Transformer.h
===
--- clang/include/clang/Tooling/Transformer/Transformer.h
+++ clang/include/clang/Tooling/Transformer/Transformer.h
@@ -22,16 +22,25 @@
 /// defined by the arguments of the constructor.
 class Transformer : public ast_matchers::MatchFinder::MatchCallback {
 public:
-  using ChangeConsumer =
-  std::function Change)>;
-
-  /// \param Consumer Receives each rewrite or error.  

[PATCH] D119745: [libTooling] Change Tranformer's consumer to take multiple changes

[Eric Li via Phabricator via cfe-commits]

li.zhe.hua updated this revision to Diff 408847.
li.zhe.hua added a comment.

Remove old constructor

Not sure how to resolve the ambiguous constructor issue on MSVC. This is
probably SFINAE-able, but not sure how to repro locally to iterate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119745

Files:
  clang/include/clang/Tooling/Transformer/Transformer.h
  clang/lib/Tooling/Transformer/Transformer.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ 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();
+  auto Factory = newFrontendActionFactory();
+  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 ) { return C.getFilePath(); },
+   "input.cc"),
+  ResultOf([](const AtomicChange ) { return C.getFilePath(); },
+   "./input.h";
+}
+
 } // namespace
Index: clang/lib/Tooling/Transformer/Transformer.cpp
===
--- clang/lib/Tooling/Transformer/Transformer.cpp
+++ clang/lib/Tooling/Transformer/Transformer.cpp
@@ -65,6 +65,9 @@
 }
   }
 
+  llvm::SmallVector Changes;
+  Changes.reserve(ChangesByFileID.size());
   for (auto  : ChangesByFileID)
-Consumer(std::move(IDChangePair.second));
+Changes.push_back(std::move(IDChangePair.second));
+  Consumer(llvm::MutableArrayRef(Changes));
 }
Index: clang/include/clang/Tooling/Transformer/Transformer.h
===
--- clang/include/clang/Tooling/Transformer/Transformer.h
+++ clang/include/clang/Tooling/Transformer/Transformer.h
@@ -22,16 +22,25 @@
 /// defined by the arguments of the constructor.
 class Transformer : public ast_matchers::MatchFinder::MatchCallback {
 public:
-  using ChangeConsumer =
-  std::function Change)>;
-
-  /// \param Consumer Receives each 

[PATCH] D119745: [libTooling] Change Tranformer's consumer to take multiple changes

[Eric Li via Phabricator via cfe-commits]

li.zhe.hua updated this revision to Diff 408699.
li.zhe.hua added a comment.

Attempt fix for Windows compilation issue

Work around ambiguous function call by foregoing the delgating constructor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119745

Files:
  clang/include/clang/Tooling/Transformer/Transformer.h
  clang/lib/Tooling/Transformer/Transformer.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ 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();
+  auto Factory = newFrontendActionFactory();
+  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 ) { return C.getFilePath(); },
+   "input.cc"),
+  ResultOf([](const AtomicChange ) { return C.getFilePath(); },
+   "./input.h";
+}
+
 } // namespace
Index: clang/lib/Tooling/Transformer/Transformer.cpp
===
--- clang/lib/Tooling/Transformer/Transformer.cpp
+++ clang/lib/Tooling/Transformer/Transformer.cpp
@@ -65,6 +65,9 @@
 }
   }
 
+  llvm::SmallVector Changes;
+  Changes.reserve(ChangesByFileID.size());
   for (auto  : ChangesByFileID)
-Consumer(std::move(IDChangePair.second));
+Changes.push_back(std::move(IDChangePair.second));
+  Consumer(llvm::MutableArrayRef(Changes));
 }
Index: clang/include/clang/Tooling/Transformer/Transformer.h
===
--- clang/include/clang/Tooling/Transformer/Transformer.h
+++ 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 

[PATCH] D119745: [libTooling] Change Tranformer's consumer to take multiple changes

[Eric Li via Phabricator via cfe-commits]

li.zhe.hua added a comment.

In D119745#3320154 , @ymandel wrote:

> Looks good. But, please add tests. Thanks!

Done; added a test for a multi-AtomicChange edit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119745

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119745: [libTooling] Change Tranformer's consumer to take multiple changes

[Eric Li via Phabricator via cfe-commits]

li.zhe.hua updated this revision to Diff 408636.
li.zhe.hua marked 2 inline comments as done.
li.zhe.hua added a comment.

Add test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119745

Files:
  clang/include/clang/Tooling/Transformer/Transformer.h
  clang/lib/Tooling/Transformer/Transformer.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ 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();
+  auto Factory = newFrontendActionFactory();
+  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 ) { return C.getFilePath(); },
+   "input.cc"),
+  ResultOf([](const AtomicChange ) { return C.getFilePath(); },
+   "./input.h";
+}
+
 } // namespace
Index: clang/lib/Tooling/Transformer/Transformer.cpp
===
--- clang/lib/Tooling/Transformer/Transformer.cpp
+++ clang/lib/Tooling/Transformer/Transformer.cpp
@@ -65,6 +65,9 @@
 }
   }
 
+  llvm::SmallVector Changes;
+  Changes.reserve(ChangesByFileID.size());
   for (auto  : ChangesByFileID)
-Consumer(std::move(IDChangePair.second));
+Changes.push_back(std::move(IDChangePair.second));
+  Consumer(llvm::MutableArrayRef(Changes));
 }
Index: clang/include/clang/Tooling/Transformer/Transformer.h
===
--- clang/include/clang/Tooling/Transformer/Transformer.h
+++ 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 

[PATCH] D119745: [libTooling] Change Tranformer's consumer to take multiple changes

[Eric Li via Phabricator via cfe-commits]

li.zhe.hua updated this revision to Diff 408587.
li.zhe.hua added a comment.

Update based on comments.

Update test to switch off depreated constructor.
Fix assert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119745

Files:
  clang/include/clang/Tooling/Transformer/Transformer.h
  clang/lib/Tooling/Transformer/Transformer.cpp
  clang/unittests/Tooling/TransformerTest.cpp


Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -120,10 +120,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: "
Index: clang/lib/Tooling/Transformer/Transformer.cpp
===
--- clang/lib/Tooling/Transformer/Transformer.cpp
+++ clang/lib/Tooling/Transformer/Transformer.cpp
@@ -65,6 +65,9 @@
 }
   }
 
+  llvm::SmallVector Changes;
+  Changes.reserve(ChangesByFileID.size());
   for (auto  : ChangesByFileID)
-Consumer(std::move(IDChangePair.second));
+Changes.push_back(std::move(IDChangePair.second));
+  Consumer(llvm::MutableArrayRef(Changes));
 }
Index: clang/include/clang/Tooling/Transformer/Transformer.h
===
--- clang/include/clang/Tooling/Transformer/Transformer.h
+++ 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  : *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


Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -120,10 +120,11 @@
 return *ChangedCode;
   }
 
-  Transformer::ChangeConsumer consumer() {
-return [this](Expected C) {
+  Transformer::ChangeSetConsumer consumer() {
+return [this](Expected> C) {
   if (C) {
-

[PATCH] D119745: [libTooling] Change Tranformer's consumer to take multiple changes

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel accepted this revision.
ymandel added a comment.
This revision is now accepted and ready to land.

Looks good. But, please add tests. Thanks!




Comment at: clang/include/clang/Tooling/Transformer/Transformer.h:27
 
+  using ChangesConsumer = std::function> Changes)>;

Maybe `ChangeSetConsumer` to be more clearly a different word than 
`ChangeConsumer`?



Comment at: clang/include/clang/Tooling/Transformer/Transformer.h:28
+  using ChangesConsumer = std::function> Changes)>;
+

Maybe document the choice of MutableArrayRef vs passing by value? I think this 
is the right choice, but it's not totally obvious. My justification is that we 
assume most consumers are reading the data and not storing the whole array.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119745

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119745: [libTooling] Change Tranformer's consumer to take multiple changes

[Eric Li via Phabricator via cfe-commits]

li.zhe.hua created this revision.
li.zhe.hua added a reviewer: ymandel.
li.zhe.hua requested review of this revision.
Herald added a project: clang.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119745

Files:
  clang/include/clang/Tooling/Transformer/Transformer.h
  clang/lib/Tooling/Transformer/Transformer.cpp


Index: clang/lib/Tooling/Transformer/Transformer.cpp
===
--- clang/lib/Tooling/Transformer/Transformer.cpp
+++ clang/lib/Tooling/Transformer/Transformer.cpp
@@ -65,6 +65,9 @@
 }
   }
 
+  llvm::SmallVector Changes;
+  Changes.reserve(ChangesByFileID.size());
   for (auto  : ChangesByFileID)
-Consumer(std::move(IDChangePair.second));
+Changes.push_back(std::move(IDChangePair.second));
+  Consumer(llvm::MutableArrayRef(Changes));
 }
Index: clang/include/clang/Tooling/Transformer/Transformer.h
===
--- clang/include/clang/Tooling/Transformer/Transformer.h
+++ clang/include/clang/Tooling/Transformer/Transformer.h
@@ -22,16 +22,38 @@
 /// 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)>;
 
+  using ChangesConsumer = 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  : *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, ChangesConsumer Consumer)
+  : Rule(std::move(Rule)), Consumer(std::move(Consumer)) {
+assert(Consumer && "Consumer is empty");
+  }
 
   /// N.B. Passes `this` pointer to `MatchFinder`.  So, this object should not
   /// be moved after this call.
@@ -43,8 +65,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.
+  ChangesConsumer Consumer;
 };
 } // namespace tooling
 } // namespace clang


Index: clang/lib/Tooling/Transformer/Transformer.cpp
===
--- clang/lib/Tooling/Transformer/Transformer.cpp
+++ clang/lib/Tooling/Transformer/Transformer.cpp
@@ -65,6 +65,9 @@
 }
   }
 
+  llvm::SmallVector Changes;
+  Changes.reserve(ChangesByFileID.size());
   for (auto  : ChangesByFileID)
-Consumer(std::move(IDChangePair.second));
+Changes.push_back(std::move(IDChangePair.second));
+  Consumer(llvm::MutableArrayRef(Changes));
 }
Index: clang/include/clang/Tooling/Transformer/Transformer.h
===
--- clang/include/clang/Tooling/Transformer/Transformer.h
+++ clang/include/clang/Tooling/Transformer/Transformer.h
@@ -22,16 +22,38 @@
 /// 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)>;
 
+  using ChangesConsumer = std::function> Changes)>;
+
+  /// Deprecated. Use the constructor accepting \c ChangesConsumer.
+  ///
   /// \param Consumer Receives each rewrite