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