This is an archive of the discontinued LLVM Phabricator instance.

[Syntax] Add mapping from spelled to expanded tokens for TokenBuffer
ClosedPublic

Authored by hlopko on Apr 1 2020, 4:02 AM.

Details

Summary

Same restrictions apply as in the other direction: macro arguments are
not supported yet, only full macro expansions can be mapped.

Taking over from https://reviews.llvm.org/D72581.

Diff Detail

Event Timeline

hlopko created this revision.Apr 1 2020, 4:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2020, 4:02 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
hlopko updated this revision to Diff 254188.Apr 1 2020, 7:21 AM

Adding comments.

gribozavr2 added inline comments.Apr 3 2020, 6:57 AM
clang/lib/Tooling/Syntax/Tokens.cpp
239

It would be great to add an is_sorted assertion to the builder to check ordering of mappings based on both spelled and expanded token indices.

259

s/subarray/subrange/

Also no need to repeat "assert".

"Spelled must be a subrange of File.SpelledTokens."

261

I think we can improve the precision of this assertion. Comparing the sizes does not ensure that we really have a subrange. I think the second assert should be comparing end pointers instead of sizes. Something like:

assert(&Spelled.back() <= &File.SpelledTokens.back());

264

Or assert that SpelledFrontI is less than File.SpelledTokens.size().

hlopko updated this revision to Diff 255326.Apr 6 2020, 7:23 AM
hlopko marked 4 inline comments as done.

Adressing comments

hlopko marked an inline comment as done.Apr 6 2020, 7:23 AM

Adressing comments.

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

I think the assert I added is good enough?

gribozavr2 added inline comments.Apr 6 2020, 8:37 AM
clang/lib/Tooling/Syntax/Tokens.cpp
264

The assertion that I suggested is stronger, because it would prevent an out-of-bounds read from even happening, and would not rely on isBeforeInTranslationUnit returning false on garbage data.

586

I'd suggest moving this loop to the very bottom of this function. It is not related to the expanded token processing, which happens here (above we have assertions related to expanded tokens, and below we have the processExpandedToken call).

Also, fillGapsAtEndOfFiles below modifies mappings, it would be good if modifications were covered by the assertion as well.

587

We'd get an "unused variable" warning here when assertions are disabled. Please wrap the whole loop in #ifndef NDEBUG.

hlopko updated this revision to Diff 255609.Apr 7 2020, 1:08 AM
hlopko marked 6 inline comments as done.

Adressing comments

hlopko added inline comments.Apr 7 2020, 1:16 AM
clang/lib/Tooling/Syntax/Tokens.cpp
264

Great points! Done.

587

Done, added ifndef also above for T1/T2 variables.

hlopko updated this revision to Diff 255654.Apr 7 2020, 5:56 AM

Assert the Spelled subrange more effectively

gribozavr2 accepted this revision.Apr 7 2020, 5:57 AM
This revision is now accepted and ready to land.Apr 7 2020, 5:57 AM
This revision was automatically updated to reflect the committed changes.