[PATCH] D120360: [libTooling] Generalize string explanation as Any metadata

[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 really good!



================
Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:64
 
 using TextGenerator = std::shared_ptr<MatchComputation<std::string>>;
 
----------------
(and move down to after `Generator` def.)


================
Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:291
 
-  /// DEPRECATED: use `::clang::transformer::RootID` instead.
-  static const llvm::StringRef RootID;
+template <typename MetadataT> struct RewriteRuleWith : RewriteRuleBase {
+  SmallVector<Generator<MetadataT>, 1> Metadata;
----------------
Please comment.


================
Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:300
 /// Constructs a simple \c RewriteRule.
 RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
+                     EditGenerator Edits);
----------------
Maybe add an adaptor for edit generator like we do in Stencil::cat. That will allow us to collapse the 6 overloads (2 * 3) into 2 (for makerule) + 3 (for the adaptor).  Only saves 1 but I think it will clearer and more flexible for the future.


================
Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:507
 
 /// Returns the \c Case of \c Rule that was selected in the match result.
 /// Assumes a matcher built with \c buildMatcher.
----------------
please update


================
Comment at: clang/include/clang/Tooling/Transformer/Transformer.h:22
+
+namespace internal_transformer {
+/// Implementation details of \c Transformer with type erasure around
----------------
in RewriteRule.h, we use "detail". Maybe do the same here?


================
Comment at: clang/include/clang/Tooling/Transformer/Transformer.h:48
+/// \c RewriteRule.
+class PlainConsumer final : public TransformerImpl {
+  transformer::RewriteRule Rule;
----------------
I'm afraid the use of the name `Consumer` (here and below) might be confused with the `Consumer` argument.  What about just `Impl` or something similar?


================
Comment at: clang/include/clang/Tooling/Transformer/Transformer.h:116
+  explicit Transformer(transformer::RewriteRuleWith<T> Rule,
+                       ConsumerFn Consumer);
 
----------------
why won't we have the same SFINAE here to ensure ConsumerFn is invocable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120360