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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
