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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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