[PATCH] D123289: [clangd][SymbolCollector] Introduce a cache for SymbolID generation and some cleanups

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:189
+  clang::Token Tok;
+  if (clang::Lexer::getRawToken(Loc, Tok, SM, LO))
+    return false;
----------------
You've changed this from tokenizing the file with a cache.
If I'm reading your benchmark spreadsheet right, this is ~3% speedup.

I'm not sure this is significant (I imagine not), but you don't actually have to run the lexer here, since you already know what the string is going to be, it's enough to grab the buffer pointer, check that it starts with the text, check that the next character is not an identifier-continuer.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:710
     // FIXME: Populate container information for macro references.
-    MacroRefs[ID].push_back({Loc, Roles, /*Container=*/nullptr});
+    // FIXME: All macro references are marked as Spelled now, but this should be
+    // checked.
----------------
kadircet wrote:
> sammccall wrote:
> > What does a non-spelled macro reference look like?
> I don't fully understand it either (hence decided to keep it as-is), but my initial guess is nested macro expansions, e.g:
> ```
> #define FOO(X) X
> #define BAR(X) FOO(X)
> 
> BAR(int x);
> ```
> 
> I suppose we assume there's a reference to FOO at expansion of BAR here today. But I am not sure if `libIndex` will actually emit a macro reference for `FOO` here.
oops nevermind, you're only moving this comment from elsewhere


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123289