Page MenuHomePhabricator

[LibTooling] Add insert/remove convenience functions for creating `ASTEdit`s.
ClosedPublic

Authored by ymandel on May 29 2019, 12:36 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

ymandel created this revision.May 29 2019, 12:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2019, 12:36 PM
ymandel updated this revision to Diff 202026.May 29 2019, 12:44 PM

added comments.

Are there any corner cases with handling whitespace that we might want to handle or document them if we don't handle them?
E.g. if the range ends up being a name of an expanded macro , it's probably very easy to glue the macro name with whatever you're inserting.

#define FOO 1+2;
#define BAR 2+3;

[[FOO]] // [[ ]] marks the selected range

would insertBefore([[FOO]], "BAR") end up being BARFOO or BAR FOO?

clang/include/clang/Tooling/Refactoring/Transformer.h
202 ↗(On Diff #202026)

Could you elaborate on what calculating the end location of \p S means?
Is the fact that the end location is calculated specific to this function, rather than RangeSelector in general?

The comment is very different from insertBefore, suggesting the function behaves differently in practice.

I guess the question also applies to change, although it did not occur to me before.

ymandel marked an inline comment as done.Jun 4 2019, 11:40 AM

Thanks for the review.

Are there any corner cases with handling whitespace that we might want to handle or document them if we don't handle them?
E.g. if the range ends up being a name of an expanded macro , it's probably very easy to glue the macro name with whatever you're inserting.

#define FOO 1+2;
#define BAR 2+3;

[[FOO]] // [[ ]] marks the selected range

would insertBefore([[FOO]], "BAR") end up being BARFOO or BAR FOO?

I guess the question also applies to change, although it did not occur to me before.

Good questions. The answer to both is that it depends on the RangeSelector used. change itself takes exactly the range given it and modifies the source at that range. Since these APIs are acting at the character level, I think it could be confusing to add token-level reasoning. That said, we could add RangeSelectors and/or TextGenerators that are smarter about this and ensure/add spaces around text as needed. Since all changes should (in my opinion) be run through clang-format afterwards, there's no risk in adding extra whitespace. For example, we could change text() to add whitespace on both sides of the argument, or add a new combinator pad which does that (and leave text() alone). So, for this example, the user would write insertBefore([[FOO]], pad("BAR")) and get BAR FOO.

We could similarly update Stencil to pad its output by default or somesuch.

WDYT?

clang/include/clang/Tooling/Refactoring/Transformer.h
202 ↗(On Diff #202026)

I'm glad you bring this up, since I struggled with how best to express this. The point is that it handles the difference between CharRange and TokenRange and does the "right" thing. This kind of thing has bitten me and others in the past -- it's a pretty typical newbie error to mishandle the SourceRange of a node.

That said, it is *not* doing anything more than RangeSelector already does, since RangeSelector is already built on CharSourceRange rather than SourceRange. So, I'd be fine dropping this mention entirely, since it's really a property of the whole system. That is, I could replace with
"Inserts \p Replacement after \p S, leaving the source selected by \S unchanged."

WDYT?

As for insertBefore -- since the beginning of a range is unambiguous, there's no need to say anything interesting there.

ilya-biryukov accepted this revision.Jun 4 2019, 11:25 PM

Thanks for the review.

Good questions. The answer to both is that it depends on the RangeSelector used. change itself takes exactly the range given it and modifies the source at that range. Since these APIs are acting at the character level, I think it could be confusing to add token-level reasoning. That said, we could add RangeSelectors and/or TextGenerators that are smarter about this and ensure/add spaces around text as needed. Since all changes should (in my opinion) be run through clang-format afterwards, there's no risk in adding extra whitespace. For example, we could change text() to add whitespace on both sides of the argument, or add a new combinator pad which does that (and leave text() alone). So, for this example, the user would write insertBefore([[FOO]], pad("BAR")) and get BAR FOO.

We could similarly update Stencil to pad its output by default or somesuch.

WDYT?

The current design sounds reasonable to me. The macro case I mentioned is somewhat weird and I'm not even sure how often it shows up in practice. Most range selectors based on AST nodes should start and end at reasonable position that don't require any extra spaces when replaced.

Let's keep the whitespace problem in mind, but not do anything about it just yet. If this actually turns out to be a problem in practice we can revisit, having some intuition what the actual tricky cases are and maybe with more approaches on how to solve them.

Also LGTM, thanks!

clang/include/clang/Tooling/Refactoring/Transformer.h
202 ↗(On Diff #202026)

I like the idea of a short comment not mentioning the token range peculiarities. At the very end, that's what everyone should expect from the API. The fact that clang's methods require this is surprising, transformers just "boringly" do the right thing.

This revision is now accepted and ready to land.Jun 4 2019, 11:25 PM
ymandel updated this revision to Diff 203122.Jun 5 2019, 4:46 AM
ymandel marked an inline comment as done.

adjusted API comments.

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