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

[Andy Soffer via Phabricator via cfe-commits]

asoffer added inline comments.


================
Comment at: clang/include/clang/Tooling/Transformer/Transformer.h:103-108
+  template <typename ConsumerFn,
+            std::enable_if_t<
+                llvm::is_invocable<
+                    ConsumerFn,
+                    llvm::Expected<llvm::MutableArrayRef<AtomicChange>>>::value,
+                int> = 0>
----------------
Given that we're simply passing this off to the NoMetadataImpl which accepts a std::function, I don't see any reason to accept a generic parameter via sfinae. We could just accept the std::function and move it. Then the implicit conversion from whatever is passed in to std::function will do the heavy sfinae lifting.


================
Comment at: clang/include/clang/Tooling/Transformer/Transformer.h:116
+  explicit Transformer(transformer::RewriteRuleWith<T> Rule,
+                       ConsumerFn Consumer);
 
----------------
li.zhe.hua wrote:
> 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?
Accepting the std:function directly should simplify this and avoid the need for the Result<void> specialization.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120360