This is an archive of the discontinued LLVM Phabricator instance.

[Syntax] expose API for expansions overlapping a spelled token range.
ClosedPublic

Authored by sammccall on Jul 17 2020, 2:35 AM.

Details

Summary

This allows efficiently accessing all expansions (without iterating over each
token and searching), and also identifying tokens within a range that are
affected by the preprocessor (which is how clangd will use it).

Diff Detail

Event Timeline

sammccall created this revision.Jul 17 2020, 2:35 AM
ArcsinX added inline comments.Jul 17 2020, 3:45 AM
clang/lib/Tooling/Syntax/Tokens.cpp
252

Could we use const auto & here as in lines 419, 434 for consistency?

431

Will it be useful to have similar API with FileID/MarkedFile parameter?
For example, we already have FileID in SelectionTester::SelectionTester.

sammccall updated this revision to Diff 278716.Jul 17 2020, 4:01 AM
sammccall marked 2 inline comments as done.

auto

clang/lib/Tooling/Syntax/Tokens.cpp
431

FileID is a bit awkward: then you have to have some way to specify which expansions you're interested in (or get those for the whole file and filter them yourself).

We could add an overload for this, but I think Buf.expansionsAffecting(Buf.spelledTokens(FID)) is pretty acceptable for this case.

(MarkedFile can't be used in an API because it's private)

Thanks. This LGTM, but I think I don't have enough experience to accept revisions and could miss something important. So, may be @kadircet could accept it if its OK for him.

thanks, mostly LG with a comment about some edge case. just wanna know what you think.

clang/include/clang/Tooling/Syntax/Tokens.h
281

this sounds more like expansionsAffected rather than affecting ? maybe my mental model requires correction, but it feels like spelled tokens are not affected by expansions, it is the other way around.

maybe even expansionsSpawned or triggered?

this is definitely non-blocking though, i am just bikesheding, comment is explanatory enough in any case.

clang/lib/Tooling/Syntax/Tokens.cpp
254

these two are also asserted in fileForSpelled

257

maybe move that into fileForSpelled too ? (to ensure Spelled is contained within the returned file)

431

this might be inconistent with spelledForExpanded from time to time, e.g:

#define FOO(X) 123
#define BAR

void foo() {
  FOO(BA^R);
}

normally BAR has no expansions, but I think it will get merged into outer macro expansion e.g. 123 coming from FOO(BAR). (whereas spelledForExpanded will treat BAR in isolation)

not sure an important limitation but something to keep in mind.

sammccall marked 4 inline comments as done.Jul 17 2020, 6:14 AM
sammccall added inline comments.
clang/include/clang/Tooling/Syntax/Tokens.h
281

Either mental model works I think. Mine is that expansions are rewrite rules - you start with a stream of tokens, and then each expansion mutates it, and you end up with different ones.

Renamed to expansionsOverlapping which seems more neutral and consistent with expansionStartingAt

clang/lib/Tooling/Syntax/Tokens.cpp
431

I don't understand the consistency you're looking for - AFAICS these are different functions that do different things - spelledForExpanded is much higher level.

Is there something in the name or comment I can add/remove to clarify?

normally BAR has no expansions

Tokens don't really "have expansions" - they're *part of* expansions. Generally expansionsOverlapping() will return the expansions themselves that are overlapping, while spelledForExpanded will include the expanded tokens if the whole expansion is in-range.

The former is a building-block (returning Expansions seems like a clear indicator of that) that will usually require some postfiltering, while the latter is fairly directly usable.

sammccall updated this revision to Diff 278748.Jul 17 2020, 6:15 AM

expansionsAffecting->expansionsOverlapping

kadircet accepted this revision.Jul 17 2020, 6:52 AM

thanks, LGTM! (and loved the choice of overlapping)

clang/lib/Tooling/Syntax/Tokens.cpp
431

Tokens don't really "have expansions" - they're *part of* expansions. Generally expansionsOverlapping() will return the expansions themselves that are overlapping, while spelledForExpanded will include the expanded tokens if the whole expansion is in-range.

yeah expansionsOverlapping() will return the expansions themselves that are overlapping bit made my reasoning a little more solid, i am still thinking with the spelled tokens might expand mindset but I suppose spelled tokens might be part of expansions is a better reasoning.

I don't think any extra comments are necessary, at least in this layer.

This revision is now accepted and ready to land.Jul 17 2020, 6:52 AM
This revision was automatically updated to reflect the committed changes.