Page MenuHomePhabricator

[LibTooling] Add RangeSelector library for defining source ranges based on bound AST nodes.
ClosedPublic

Authored by ymandel on May 9 2019, 6:35 PM.

Details

Summary

The RangeSelector library defines a combinator language for specifying source
ranges based on bound id for AST nodes. The combinator approach follows the
design of the AST matchers. The RangeSelectors defined here will be used in
both RewriteRule, for specifying source affected by edit, and in Stencil for
specifying source to use constructively in a replacement.

Diff Detail

Repository
rL LLVM

Event Timeline

ymandel created this revision.May 9 2019, 6:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2019, 6:35 PM
Herald added a subscriber: mgorny. · View Herald Transcript

Ilya, this revision follows up from our discussion on the doc. It adds some more selectors. If this change is to big, I'm happy to split it up (e.g. moving the changes to SourceCode to a separate revision and/or splitting up the RangeSelector changes). Also, I have the diff updating Transformer ready if you want to see that. I can post a preview or created a stacked revision. Let me know...

Thanks! Mostly NITs from me, the design looks nice! Would you mind adding some tests before we land this?

The only major thing that I'd argue against is adding helper functions like findPreviousToken to SourceCode.h.
It's fine to have them in the .cpp files if they make the implementation simpler, but we want to design them more carefully if we put them in the public API.

clang/include/clang/Tooling/Refactoring/RangeSelector.h
29 ↗(On Diff #198969)

Maybe try putting it into the tooling namespace directly?
Or are there immediate and unfortunate conflicts with existing names?

35 ↗(On Diff #198969)

NIT: maybe explain what Id is?

36 ↗(On Diff #198969)

NIT: I believe LLVM naming rules would mandate to name it ID

37 ↗(On Diff #198969)

Maybe a shorter description could suffice?

Selects a node and a trailing semicolon. Useful for selecting expression statements.
43 ↗(On Diff #198969)

NIT: maybe name it statement?

46 ↗(On Diff #198969)

Could this be an overload with the same name?

53 ↗(On Diff #198969)

NIT: this is super-specialized, but has a very generic name, therefore might cause confusion. Maybe call it ctorInitializerName?

57 ↗(On Diff #198969)

Same thing, maybe rename to callExprArgs?
And other nodes too

74 ↗(On Diff #198969)

NIT: maybe call it expansion?

contraction is a new term, might be confusing to use.

clang/include/clang/Tooling/Refactoring/SourceCode.h
76 ↗(On Diff #198969)

Not sure this should be a public API. Maybe keep it private to the use-site?

ymandel updated this revision to Diff 199894.May 16 2019, 1:47 PM
ymandel marked 3 inline comments as done.

Add tests and back out changes to SourceCode.

ymandel updated this revision to Diff 199896.May 16 2019, 1:50 PM
ymandel marked 9 inline comments as done.

comment tweak

ymandel edited the summary of this revision. (Show Details)May 16 2019, 1:50 PM
ymandel marked 3 inline comments as done and an inline comment as not done.May 16 2019, 1:53 PM

Thanks for the review. Added the tests and undid changes to SourceCode.

clang/include/clang/Tooling/Refactoring/RangeSelector.h
29 ↗(On Diff #198969)

No conflicts. Was just being cautious.

36 ↗(On Diff #198969)

here and throughout.

53 ↗(On Diff #198969)

It works with others as well, particularly NamedDecl. This is one place where typed node ids would be nice, b/c that would allow us to make this interface typed, like the matchers.

I kept the name but tried to clarify the comments. WDYT?

57 ↗(On Diff #198969)

i'd like to keep these as lightweight as possible. Compromised on callArgs?

74 ↗(On Diff #198969)

Sigh. You're right. Its just that the existing terminology is confusing. that said, anyone using this combinator is already doing advanced work, so consistency is probably more important than clarity.

clang/include/clang/Tooling/Refactoring/SourceCode.h
76 ↗(On Diff #198969)

Good point. Originally, we needed these in two different files -- but the range selectors actually obviate that.

ilya-biryukov marked an inline comment as done.May 17 2019, 2:05 AM
ilya-biryukov added inline comments.
clang/include/clang/Tooling/Refactoring/RangeSelector.h
29 ↗(On Diff #198969)

I can see why a separate namespace would make sense, but since the tooling namespace is not densely populated I hope we can get away with putting things here directly and saving some typing on the use-sites.

Hope that does not backfire in the future, though.

53 ↗(On Diff #198969)

Ah, sorry, misread the original comment. The name actually makes sense in that case.
Am I correct to assume it will only select the final identifier of a qualified name, but not the qualifier?
E.g. for ::foo::bar::baz, it would only select baz, right?
What happens when we also have template args? E.g. for ::foo::bar::baz<int>, it would only select baz, right?

Maybe add those examples to the documentation? It's something that's very easy to get wrong.

57 ↗(On Diff #198969)

callArgs LG, thanks!

clang/unittests/Tooling/RangeSelectorTest.cpp
39 ↗(On Diff #199896)

NIT: use ASTUnit to match LLVM naming rules

143 ↗(On Diff #199896)

Consider restructuring the code to place assertions into the test function itself.
This wildly improves locations reported in case of test failures and makes tests easier to read.

I.e. having something like

auto AST = buildASTAndMatch(Code, Matcher);
EXPECT_EQ(applySelector(range(Arg0, Arg1), AST),  "3, 7");

is arguably both easier to read and produces better location information on failures than a function that runs the test.
Even though it's a bit more code.

Note that it's a bit more complicated if you need to deal with Expected<> return types, but still possible:

EXPECT_THAT_EXPECTED(applySelector(range(Arg0, Arg1), AST),  HasValue("3, 7"));
EXPECT_THAT_EXPECTED(applySelector(range(Arg1, Arg0), AST),  Failed());
ymandel updated this revision to Diff 200061.May 17 2019, 9:51 AM
ymandel marked 4 inline comments as done.

Restructured tests to simplify.

ymandel marked 7 inline comments as done.May 17 2019, 9:57 AM
ymandel added inline comments.
clang/include/clang/Tooling/Refactoring/RangeSelector.h
29 ↗(On Diff #198969)

SGTM.

clang/unittests/Tooling/RangeSelectorTest.cpp
143 ↗(On Diff #199896)

Thanks for the suggestion. Thoroughly reworked the tests along these lines. I think the result is much clearer.

ymandel updated this revision to Diff 200079.May 17 2019, 12:21 PM
ymandel marked 2 inline comments as done.

fixed formatting.

ilya-biryukov accepted this revision.Mon, May 20, 5:19 AM

Thanks! LGTM!

This revision is now accepted and ready to land.Mon, May 20, 5:19 AM
ymandel updated this revision to Diff 200262.Mon, May 20, 6:06 AM

updated some comments.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMon, May 20, 6:13 AM

Test file broke gcc builds. Fix in progress...

Fixed with r361160.

penzn added a subscriber: penzn.Wed, May 22, 1:31 PM
penzn added inline comments.
cfe/trunk/lib/Tooling/Refactoring/RangeSelector.cpp
229

Sorry for posting here, haven't gotten my bugzilla access yet (requested though).

This breaks with Visual Studio 2017 (15.7.6):

RangeSelector.cpp(229): error C2971: '`anonymous-namespace'::RelativeSelector': template parameter 'Func': 'getStatementsRange': a variable with non-static storage duration cannot be used as a non-type argument

penzn marked an inline comment as done.Thu, May 23, 10:38 AM
penzn added inline comments.
cfe/trunk/lib/Tooling/Refactoring/RangeSelector.cpp
229