[PATCH] D74842: [clangd] Make use of syntax tokens in ReplayPreamble

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:138
+      : Includes(Includes), Delegate(Delegate), SM(SM), PP(PP) {
+    MainFileTokens = syntax::tokenize(SM.getMainFileID(), SM, LangOpts);
+  }
----------------
tokenizing the whole file an extra time on every AST build seems a bit sad - this is considerably more lexing than we were doing before. Probably doesn't matter?

We could trim this to the preamble bounds I guess. Or even compute it once when the preamble is built, since we assume all the bytes are the same? I guess SourceLocations are the problem... we could just translate offsets into the new SM, but that gets messy.
On the other hand, assuming the preamble isn't going to change at all seems like an assumption not long for this world.
On the first hand again, maybe we'll have to revisit looots of stuff (go to definition and everything) once that assumption breaks anyway.


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:173
+      auto HashLoc = SM.getComposedLoc(SM.getMainFileID(), Inc.HashOffset)
+                         .getRawEncoding();
+      auto HashTok =
----------------
why raw encoding?


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:175
+      auto HashTok =
+          llvm::find_if(MainFileTokens, [&HashLoc](const syntax::Token &T) {
+            return T.location().getRawEncoding() == HashLoc;
----------------
this looks like a linear search for each #include


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:186
+
+      // We imitate the PP logic here, except clang::Token::Flags, none of the
+      // callers seem to care about it (yet).
----------------
Not clear what "imitate the PP logic" means.
We construct a fake 'import'/'include' token... nobody cares about clang::Token::Flags.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74842