ymandel marked 2 inline comments as done.
ymandel added inline comments.
================
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)));
----------------
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.).
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
