[PATCH] D62149: [LibTooling] Update Transformer to use RangeSelector instead of NodePart enum.

[Ilya Biryukov via Phabricator via cfe-commits]

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