[PATCH] D76125: [clangd] Decouple preambleworker from astworker, NFCI

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 16 inline comments as done.
kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:265
+  /// Updates the TUStatus and emits it. Only called in the worker thread.
+  void emitTUStatus(TUAction Action,
+                    const TUStatus::BuildDetails *Details = nullptr) {
----------------
sammccall wrote:
> as discussed a bit in chat, I think this part needs some more thought. Currently the ASTWorker and PreambleWorker emit data for the same file with some interleaving that's hard to reason about. The state needs an owner.
> 
> (e.g. consider a class that holds a TUStatus and exposes an Event<TUStatus>, with threadsafe setPreambleAction and setWorkerAction methods, or so)
sent out https://reviews.llvm.org/D76304


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:600
         Inputs, CompilerInvocationDiagConsumer, &CC1Args);
+    auto OldPreamble = PW.getLatestBuiltPreamble();
+    PW.requestBuild(Invocation.get(), Inputs);
----------------
sammccall wrote:
> kadircet wrote:
> > sammccall wrote:
> > > this doesn't seem correct (maybe ok in this patch because of the blocking, but not in general). You're assuming the last available preamble is the one that the last AST was built with.
> > > 
> > > I suppose you can't check the preamble of the current ParsedAST because it might not be cached, and you nevertheless want to skip rebuild if the diagnostics are going to be the same. I can't think of anything better than continuing to hold the shared_ptr for PreambleForLastBuiltAST or something like that.
> > right, this is just a "hack" to keep this change NFC.
> > 
> > in the follow-up patches i am planning to signal whether latest built preamble is reusable for a given `ParseInputs`, and also signal what the AST should be patched with.
> > 
> > diagnostics(ast) will only be built if preamble is re-usable.
> > right, this is just a "hack" to keep this change NFC.
> 
> can you add a comment about this? (In particular, why it's correct?)
> 
> > in the follow-up patches i am planning to signal whether latest built preamble is reusable for a given ParseInputs
> 
> Does "reusable" mean "completely valid" (current semantics), or usable with some tweaks (e.g. added headers)? It would be good to define some precise terminology around this.
> 
> > diagnostics(ast) will only be built if preamble is re-usable.
> 
> SG
reusable means completely valid.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:412
     RanASTCallback = false;
-    emitTUStatus({TUAction::BuildingPreamble, TaskName});
     log("ASTWorker building file {0} version {1} with command {2}\n[{3}]\n{4}",
----------------
sammccall wrote:
> why are we moving this?
it has been moved into PreambleThread instead, same as the one below handling the failure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76125