[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<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: "
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<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));
 }
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<void(Expected<clang::tooling::AtomicChange> Change)>;
+  using ChangeConsumer = std::function<void(Expected<AtomicChange> 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<void(
+      Expected<llvm::MutableArrayRef<AtomicChange>> 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<llvm::MutableArrayRef<AtomicChange>> 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<AtomicChange>.
+  ChangeSetConsumer Consumer;
 };
 } // namespace tooling
 } // namespace clang


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D119745.408587.patch
Type: text/x-patch
Size: 4331 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220214/5c4d3b31/attachment.bin>