ilya-biryukov accepted this revision.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM. See the nit about the comment, might be a good idea to fix it before
landing.
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:186
+/// where a \c TextGenerator, \c RangeSelector are otherwise expected.
+inline ASTEdit change(RangeSelector Target, std::string Replacement) {
+ return change(std::move(Target), text(std::move(Replacement)));
----------------
ymandel wrote:
> ilya-biryukov wrote:
> > The overloads create quite a lot of noise (and we are already starting to
> > see a combinatorial explosion with two classes).
> >
> > Could we try to overcome this? I see two options:
> > 1. Turn `TextGenerator` and `RangeSelector` into classes, which are
> > constructible from `string` or `function<string(MatchResult)>`.
> > 2. Remove the string overloads, force users to explicitly construct the
> > parameters.
> >
> > Option (2) actually is simple and would add a bit of type safety, but will
> > make some client code a bit less readable. Do you think the readability hit
> > is significant?
> Agreed. I wasn't loving all the overloads myself. I removed all the
> string-related overloads. left the one overload that lets the user implicitly
> specify the entirety of the match.
>
> I'm ok with the result. Certainly for RangeSelector, it forces a consistency
> which I like. For the use of text() -- I could go both ways, but i think pure
> text replacements will be rare outside of tests, so I'm not sure users will
> be terribly inconvenienced by this.
>
> The only overload i'm unsure about is the one I left, because I think a) it
> will be a common case and b) it will confuse users to learn to use
> `RewriteRule::RootID`. That said, if you have an elegant, but explicit,
> alternative I'm interested. We could, for example, add combinators
> `matchedNode()` and `matchedStatement()`, defined respectively as
> `node(RewriteRule::RootID)` and `statement(RewriteRule::RootID)`. Or, add
> a`makeRule` overload for the `node()` case (and let users be explicit when
> they want the `statement()` case.).
This one overload actually looks ok and I also agree it'll probably be somewhat
common.
Thanks!
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:184
+
+/// Replaces the matched portion of a RewriteRule with \p Replacement
+inline ASTEdit change(TextGenerator Replacement) {
----------------
NIT: maybe clarify that "matched portion" is the whole match of a rewrite rule.
It looks like "portion" in the previous overload means "range defined by the
selector", while in this overload it means "the whole matched node". It'd be
nice to use different words there to avoid confusion and make the comment more
useful
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62149/new/
https://reviews.llvm.org/D62149
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
