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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 7 inline comments as done.
kadircet 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);
+  }
----------------
sammccall wrote:
> 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.
Implemented a way to partially tokenize a file in D74962.

> On the other hand, assuming the preamble isn't going to change at all seems like an assumption not long for this world.

It should be okay for replaypreambles as only clang tidy checkers depends on this logic and we are not planning to emit diagnostics with stale preambles.


================
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;
----------------
sammccall wrote:
> this looks like a linear search for each #include
made it logarithmic instead, we can also make it linear in total if we decide to rely on the fact that `MainFileIncludes` are sorted. I believe it is currently true but never promised by the include collector.


================
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).
----------------
sammccall wrote:
> Not clear what "imitate the PP logic" means.
> We construct a fake 'import'/'include' token... nobody cares about clang::Token::Flags.
it was refering to the fact that we were performing the `PP.LookupIdentifierInfo` call to set kind etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74842