This is an archive of the discontinued LLVM Phabricator instance.

[LibTooling] Update Transformer to use RangeSelector instead of NodePart enum.
ClosedPublic

Authored by ymandel on May 20 2019, 8:40 AM.

Event Timeline

ymandel created this revision.May 20 2019, 8:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2019, 8:40 AM
ymandel updated this revision to Diff 200310.May 20 2019, 8:42 AM

reordered some declarations.

Harbormaster completed remote builds in B32156: Diff 200310.

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:

  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?

ymandel updated this revision to Diff 200325.May 20 2019, 10:07 AM

Removed most change overloads.

ymandel marked 2 inline comments as done.May 20 2019, 10:17 AM
ymandel added inline comments.
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.).

ilya-biryukov accepted this revision.May 22 2019, 3:15 AM
ilya-biryukov marked an inline comment as done.

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.
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

209

This one overload actually looks ok and I also agree it'll probably be somewhat common.
Thanks!

This revision is now accepted and ready to land.May 22 2019, 3:15 AM
ymandel updated this revision to Diff 200712.May 22 2019, 5:44 AM
ymandel marked an inline comment as done.

Updated comments.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 22 2019, 7:46 AM