This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Get rid of getTokenRange helper
ClosedPublic

Authored by kadircet on Mar 2 2020, 11:11 AM.

Diff Detail

Event Timeline

kadircet created this revision.Mar 2 2020, 11:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2020, 11:12 AM
kadircet updated this revision to Diff 247808.Mar 3 2020, 12:09 AM
  • Also get rid of the call in collectmacros by using clang::Token's endlocation instead.
kadircet edited the summary of this revision. (Show Details)Mar 3 2020, 12:09 AM
sammccall added inline comments.Mar 3 2020, 12:41 AM
clang-tools-extra/clangd/Hover.cpp
541

nit: back() would fit better with the biases elsewhere

clang-tools-extra/clangd/XRefs.cpp
157–158

so this is just running the raw lexer...

both callers have a ParsedAST, can we call spelledTokenAt()? Either here, passing in (TokenBuffer, Loc, TUPath) or in the caller, passing in (syntax::Token*, SourceMgr, TUPath).

157–158

It *seems* to always be the case that TokLoc is a spelled token, but this seems slightly subtle and the previous code defended against marco locations. Worth a comment

379

document spelled or expanded.

It seems to be spelled, but I don't know why - expanded is more similar to the old code, allows us to defer more work, and doesn't require the dubious front().

You could add a function here range(SourceMgr) --> Range

425

why is it safe to discard the rest of toks?

sammccall added inline comments.Mar 3 2020, 1:47 AM
clang-tools-extra/clangd/XRefs.cpp
157–158

can we call spelledTokenAt

as you pointed out offline, we only have spelled tokens for the main file.
I think we have to lex here to get the token length, and the clearest thing is to do this completely directly (Lexer::measureTokenLength -> Loc.getLocWithOffset) rather than obfuscating with tokenbuffer.
We should have a comment *why* we lex here.

kadircet updated this revision to Diff 247849.Mar 3 2020, 4:06 AM
kadircet marked 8 inline comments as done.
  • Use getFileLoc instead of spelledForExpanded
clang-tools-extra/clangd/XRefs.cpp
157–158

so this is just running the raw lexer...
both callers have a ParsedAST, can we call spelledTokenAt()? Either here, passing in (TokenBuffer, Loc, TUPath) or in the caller, passing in (syntax::Token*, SourceMgr, TUPath).

as discussed offline the problem is TokenBuffer won't hold spelled tokens for included files. so if you've got a macro definition or declaration coming from a header, spelledTokenAt will fail.
making use of Lexer::MeasureTokenLength instead.

157–158

as discussed offline, in case of macro locations we'll fail at getting the file path and stop there and that logic is the same.

sammccall accepted this revision.Mar 3 2020, 4:46 AM
sammccall added inline comments.
clang-tools-extra/clangd/XRefs.cpp
170

(nit: consider extracting a variable for end loc or token length just for readability)

470

Ref.range(SM)

546

Ref.range(SM)

This revision is now accepted and ready to land.Mar 3 2020, 4:46 AM
kadircet updated this revision to Diff 247861.Mar 3 2020, 5:25 AM
kadircet marked 3 inline comments as done.
  • Address comments
This revision was automatically updated to reflect the committed changes.