[libTooling] Support TransformerResult<void> in consumer callbacks

[li.zhe.hua]

Summary

Support TransformerResult<void> in the consumer callback, which
allows generic code to more naturally use the Transformer interface
(instead of needing to specialize on void).

This also delete the specialization that existed within Transformer
itself, instead replacing it with an std::function adapter.

[li.zhe.hua]

Fix comment.

[ymandel]

clang/include/clang/Tooling/Transformer/Transformer.h

126–128

Why won't a simple specialization work?

189–191

simplify to else if ?

[li.zhe.hua]

Fix for comments.

[li.zhe.hua]

clang/include/clang/Tooling/Transformer/Transformer.h

126–128

Yup, totally. Done.

I guess I see SFINAE so much more often these days I just reach for it first.

clang/include/clang/Tooling/Transformer/Transformer.h

 #include <functional>
 #include <utility>
 
 namespace clang {
 namespace tooling {
 
 namespace detail {
 /// Implementation details of \c Transformer with type erasure around
-/// \c RewriteRule and \c RewriteRule<T> as well as the corresponding consumers.
+/// \c RewriteRule<T> as well as the corresponding consumers.
 class TransformerImpl {
 public:
   virtual ~TransformerImpl() = default;
 
   void onMatch(const ast_matchers::MatchFinder::MatchResult &Result);
 
   virtual std::vector<ast_matchers::internal::DynTypedMatcher>
   buildMatchers() const = 0;
 
 protected:
   /// Converts a set of \c Edit into a \c AtomicChange per file modified.
   /// Returns an error if the edits fail to compose, e.g. overlapping edits.
   static llvm::Expected<llvm::SmallVector<AtomicChange, 1>>
   convertToAtomicChanges(const llvm::SmallVectorImpl<transformer::Edit> &Edits,
                          const ast_matchers::MatchFinder::MatchResult &Result);
 
 private:
   virtual void
   onMatchImpl(const ast_matchers::MatchFinder::MatchResult &Result) = 0;
 };
 
-/// Implementation for when no metadata is generated as a part of the
-/// \c RewriteRule.
-class NoMetadataImpl final : public TransformerImpl {
-  transformer::RewriteRule Rule;
-  std::function<void(Expected<llvm::MutableArrayRef<AtomicChange>>)> Consumer;
-
-public:
-  explicit NoMetadataImpl(
-      transformer::RewriteRule R,
-      std::function<void(Expected<llvm::MutableArrayRef<AtomicChange>>)>
-          Consumer)
-      : Rule(std::move(R)), Consumer(std::move(Consumer)) {
-    assert(llvm::all_of(Rule.Cases,
-                        [](const transformer::RewriteRule::Case &Case) {
-                          return Case.Edits;
-                        }) &&
-           "edit generator must be provided for each rule");
-  }
-
-private:
-  void onMatchImpl(const ast_matchers::MatchFinder::MatchResult &Result) final;
-  std::vector<ast_matchers::internal::DynTypedMatcher>
-  buildMatchers() const final {
-    return transformer::detail::buildMatchers(Rule);
-  }
-};
-
 // FIXME: Use std::type_identity or backport when available.
 template <class T> struct type_identity {
   using type = T;
 };
 } // namespace detail
 
 template <typename T> struct TransformerResult {
   llvm::MutableArrayRef<AtomicChange> Changes;
   T Metadata;
 };
 
-// Specialization provided only to avoid SFINAE on the Transformer
-// constructor; not intended for use.
 template <> struct TransformerResult<void> {
   llvm::MutableArrayRef<AtomicChange> Changes;
 };
 
 /// Handles the matcher and callback registration for a single `RewriteRule`, as
 /// defined by the arguments of the constructor.
 class Transformer : public ast_matchers::MatchFinder::MatchCallback {
 public:
   /// 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.
   explicit Transformer(transformer::RewriteRuleWith<void> Rule,
                        ChangeSetConsumer Consumer)
-      : Impl(std::make_unique<detail::NoMetadataImpl>(std::move(Rule),
-                                                      std::move(Consumer))) {}
+      : Transformer(std::move(Rule),
+                    [Consumer = std::move(Consumer)](
+                        llvm::Expected<TransformerResult<void>> Result) {
+                      if (Result)
+                        Consumer(Result->Changes);
+                      else
+                        Consumer(Result.takeError());
+                    }) {}
 
   /// \param Consumer receives all rewrites and the associated metadata for a
   /// single match, or an error. Will always be called for each match, even if
   /// the rule generates no edits.  Note that clients are responsible for
   /// handling the case that independent \c AtomicChanges conflict with each
   /// other.
   template <typename MetadataT>
   explicit Transformer(
public:
   /// pointer.
   void run(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   std::unique_ptr<detail::TransformerImpl> Impl;
 };
 
 namespace detail {
+/// Asserts that all \c Metadata for the \c Rule is set.
+/// FIXME: Use constexpr-if in C++17.
+/// @{
+template <typename T>
+void assertMetadataSet(const transformer::RewriteRuleWith<T> &Rule) {
+  assert(llvm::all_of(Rule.Metadata,
+                      [](const typename transformer::Generator<T> &Metadata)
+                          -> bool { return !!Metadata; }) &&
+         "metadata generator must be provided for each rule");
+}
+template <>
+inline void assertMetadataSet(const transformer::RewriteRuleWith<void> &) {}
+/// @}
+
+/// Runs the metadata generator on \c Rule and stuffs it into \c Result.
+/// FIXME: Use constexpr-if in C++17.
+/// @{
+template <typename T>
+llvm::Error
+populateMetadata(const transformer::RewriteRuleWith<T> &Rule,
+                 size_t SelectedCase,
+                 const ast_matchers::MatchFinder::MatchResult &Match,
+                 TransformerResult<T> &Result) {
+  auto Metadata = Rule.Metadata[SelectedCase]->eval(Match);
+  if (!Metadata)
+    return Metadata.takeError();
+  Result.Metadata = std::move(*Metadata);
+  return llvm::Error::success();
+}
+template <>
+inline llvm::Error
+populateMetadata(const transformer::RewriteRuleWith<void> &, size_t,
+                 const ast_matchers::MatchFinder::MatchResult &Match,
+                 TransformerResult<void> &) {
+  return llvm::Error::success();
+}
+/// @}
+
 /// Implementation when metadata is generated as a part of the rewrite. This
 /// happens when we have a \c RewriteRuleWith<T>.
 template <typename T> class WithMetadataImpl final : public TransformerImpl {
   transformer::RewriteRuleWith<T> Rule;
   std::function<void(llvm::Expected<TransformerResult<T>>)> Consumer;
 
 public:
   explicit WithMetadataImpl(
       transformer::RewriteRuleWith<T> R,
       std::function<void(llvm::Expected<TransformerResult<T>>)> Consumer)
       : Rule(std::move(R)), Consumer(std::move(Consumer)) {
     assert(llvm::all_of(Rule.Cases,
                         [](const transformer::RewriteRuleBase::Case &Case)
                             -> bool { return !!Case.Edits; }) &&
            "edit generator must be provided for each rule");
-    assert(llvm::all_of(Rule.Metadata,
-                        [](const typename transformer::Generator<T> &Metadata)
-                            -> bool { return !!Metadata; }) &&
-           "metadata generator must be provided for each rule");
+    assertMetadataSet(Rule);
   }
 
 private:
   void onMatchImpl(const ast_matchers::MatchFinder::MatchResult &Result) final {
     size_t I = transformer::detail::findSelectedCase(Result, Rule);
     auto Transformations = Rule.Cases[I].Edits(Result);
     if (!Transformations) {
       Consumer(Transformations.takeError());
       return;
     }
 
     llvm::SmallVector<AtomicChange, 1> Changes;
     if (!Transformations->empty()) {
       auto C = convertToAtomicChanges(*Transformations, Result);
       if (C) {
         Changes = std::move(*C);
       } else {
         Consumer(C.takeError());
         return;
       }
+    } else if (std::is_void<T>::value) {
+      // If we don't have metadata and we don't have any edits, skip.
+      return;
     }
 
-    auto Metadata = Rule.Metadata[I]->eval(Result);
-    if (!Metadata) {
-      Consumer(Metadata.takeError());
+    TransformerResult<T> RewriteResult;
+    if (auto E = populateMetadata(Rule, I, Result, RewriteResult)) {
+      Consumer(std::move(E));
       return;
     }
 
-    Consumer(TransformerResult<T>{llvm::MutableArrayRef<AtomicChange>(Changes),
-                                  std::move(*Metadata)});
+    RewriteResult.Changes = llvm::MutableArrayRef<AtomicChange>(Changes);
+    Consumer(std::move(RewriteResult));
   }
 
   std::vector<ast_matchers::internal::DynTypedMatcher>
   buildMatchers() const final {
     return transformer::detail::buildMatchers(Rule);
   }
 };
 } // namespace detail

clang/lib/Tooling/Transformer/Transformer.cpp

TransformerImpl::convertToAtomicChanges(
   llvm::SmallVector<AtomicChange, 1> Changes;
   Changes.reserve(ChangesByFileID.size());
   for (auto &IDChangePair : ChangesByFileID)
     Changes.push_back(std::move(IDChangePair.second));
 
   return Changes;
 }
 
-void NoMetadataImpl::onMatchImpl(const MatchFinder::MatchResult &Result) {
-  size_t I = transformer::detail::findSelectedCase(Result, Rule);
-  auto Transformations = Rule.Cases[I].Edits(Result);
-  if (!Transformations) {
-    Consumer(Transformations.takeError());
-    return;
-  }
-
-  if (Transformations->empty())
-    return;
-
-  auto Changes = convertToAtomicChanges(*Transformations, Result);
-  if (!Changes) {
-    Consumer(Changes.takeError());
-    return;
-  }
-
-  Consumer(llvm::MutableArrayRef<AtomicChange>(*Changes));
-}
-
 } // namespace detail
 
 void Transformer::registerMatchers(MatchFinder *MatchFinder) {
   for (auto &Matcher : Impl->buildMatchers())
     MatchFinder->addDynamicMatcher(Matcher, this);
 }
 
 void Transformer::run(const MatchFinder::MatchResult &Result) {
   if (Result.Context->getDiagnostics().hasErrorOccurred())
     return;
 
   Impl->onMatch(Result);
 }
 
 } // namespace tooling
 } // namespace clang