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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Looks neat! The only concern I have is about the growing number of overloads in `Transformer.h`, see the relevant comment.



================
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)));
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62149/new/

https://reviews.llvm.org/D62149