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

[Eric Li via Phabricator via cfe-commits]

li.zhe.hua added inline comments.


================
Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:300
 /// Constructs a simple \c RewriteRule.
 RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
+                     EditGenerator Edits);
----------------
ymandel wrote:
> 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.
So, I ended up needing to add an overload for `std::initializer_list` as well, as it was somewhat common for people to do something like


```
makeRule(matchers(), {doThing1(), doThing2()})
```

which would have gone to the `llvm::SmallVector<ASTEdit, 1>` overload.


================
Comment at: clang/include/clang/Tooling/Transformer/Transformer.h:48
+/// \c RewriteRule.
+class PlainConsumer final : public TransformerImpl {
+  transformer::RewriteRule Rule;
----------------
ymandel wrote:
> 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?
I went with `NoMetadataImpl` (to be more descriptive than "plain") and `WithMetadataImpl`.


================
Comment at: clang/include/clang/Tooling/Transformer/Transformer.h:116
+  explicit Transformer(transformer::RewriteRuleWith<T> Rule,
+                       ConsumerFn Consumer);
 
----------------
ymandel wrote:
> why won't we have the same SFINAE here to ensure ConsumerFn is invocable?
I can't figure out how to implement the SFINAE without instantiating `Result<void>` (which is invalid because `T Metadata` is illegal then). My current attempt is

```
  template <
      typename T, typename ConsumerFn,
      std::enable_if_t<
          std::conjunction<
              std::negation<std::is_same<T, void>>,
              llvm::is_invocable<ConsumerFn, llvm::Expected<Result<T>>>>::value,
          int> = 0>
  explicit Transformer(transformer::RewriteRuleWith<T> Rule,
                       ConsumerFn Consumer);
```

I suppose I could provide a specialization of `Result<void>` that is valid? I guess I'll go with that, and just document why it exists?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120360