[PATCH] D75474: [clangd] Get rid of getTokenRange helper

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:542
+  CharSourceRange HighlightRange =
+      TokensTouchingCursor.front().range(SM).toCharRange(SM);
   llvm::Optional<HoverInfo> HI;
----------------
nit: back() would fit better with the biases elsewhere


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:155
   const SourceManager &SourceMgr = AST.getSourceManager();
-  const FileEntry *F = SourceMgr.getFileEntryForID(SourceMgr.getFileID(TokLoc));
+  auto Tok = syntax::tokenize(syntax::FileRange(SourceMgr, TokLoc, 1),
+                              SourceMgr, AST.getLangOpts());
----------------
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).


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:155
   const SourceManager &SourceMgr = AST.getSourceManager();
-  const FileEntry *F = SourceMgr.getFileEntryForID(SourceMgr.getFileID(TokLoc));
+  auto Tok = syntax::tokenize(syntax::FileRange(SourceMgr, TokLoc, 1),
+                              SourceMgr, AST.getLangOpts());
----------------
sammccall wrote:
> 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).
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


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:355
   struct Reference {
-    SourceLocation Loc;
+    syntax::Token Tok;
     index::SymbolRoleSet Role;
----------------
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


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:396
+    if (!Toks || Toks->empty() ||
+        !isInsideMainFile(Toks->front().location(), SM))
+      return true;
----------------
why is it safe to discard the rest of toks?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75474/new/

https://reviews.llvm.org/D75474