Transformer provides an enum to indicate the range of source text to be edited.
That support is now redundant with the new (and more general) RangeSelector
library, so we remove the custom enum support in favor of supporting any
RangeSelector.
Details
- Reviewers
ilya-biryukov - Commits
- rZORG99bc13e4e2d7: [LibTooling] Update Transformer to use RangeSelector instead of NodePart enum.
rG99bc13e4e2d7: [LibTooling] Update Transformer to use RangeSelector instead of NodePart enum.
rG3ec50e292f35: [LibTooling] Update Transformer to use RangeSelector instead of NodePart enum.
rC361392: [LibTooling] Update Transformer to use RangeSelector instead of NodePart enum.
rL361392: [LibTooling] Update Transformer to use RangeSelector instead of NodePart enum.
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 32155 Build 32154: arc lint + arc unit
Event Timeline
Looks neat! The only concern I have is about the growing number of overloads in Transformer.h, see the relevant comment.
clang/include/clang/Tooling/Refactoring/Transformer.h | ||
---|---|---|
209 | 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:
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? |
clang/include/clang/Tooling/Refactoring/Transformer.h | ||
---|---|---|
209 | 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.). |
LGTM. See the nit about the comment, might be a good idea to fix it before landing.
clang/include/clang/Tooling/Refactoring/Transformer.h | ||
---|---|---|
207 | NIT: maybe clarify that "matched portion" is the whole match of a rewrite rule. | |
209 | This one overload actually looks ok and I also agree it'll probably be somewhat common. |
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