[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:218
+        PreambleRequested.wait(Lock, [this] { return PreambleReq || Done; });
+        // No request means we are done.
+        if (!PreambleReq)
----------------
sammccall wrote:
> Why do we rebuild the preamble if stop is requested?
it was to be consistent with astworker, but i suppose it isn't necessary as ASTWorker does that to make sure each LSP request gets a reply, and it can be done with stale preambles.

Are there any other reasons for ASTWorker to empty the queue after stop?


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:295
+    std::shared_ptr<const PreambleData> OldPreamble =
+        Inputs.ForceRebuild ? std::shared_ptr<const PreambleData>()
+                            : getLatestBuiltPreamble();
----------------
sammccall wrote:
> (nullptr should work here?)
right, it was copied over :/


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:312
+
+  mutable std::mutex Mutex;
+  bool Done = false;
----------------
sammccall wrote:
> Yikes, lots of state :-)
> I'd consider merging the two CVs - I find keeping the events separate and reasoning when you need notify_one vs _all doesn't seem to pay off, may just be the way I think about queues. Up to you.
i like them to be seperate as it makes naming easier :D but i agree that having less state also makes reasoning easier.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:600
         Inputs, CompilerInvocationDiagConsumer, &CC1Args);
+    auto OldPreamble = PW.getLatestBuiltPreamble();
+    PW.requestBuild(Invocation.get(), Inputs);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76125