[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:623
   std::string TaskName = llvm::formatv("Update ({0})", Inputs.Version);
   auto Task = [=]() mutable {
     auto RunPublish = [&](llvm::function_ref<void()> Publish) {
----------------
sammccall wrote:
> sammccall wrote:
> > this function is getting too long/monolithic, we should find a way to split it up.
> > ASTTask should be a member function, I think.
> I also lost track of the control flow halfway through the function, and can't work it out.
> I don't know really what to concretely advise, but it needs to be a lot simpler, maybe by finding some better abstractions or just better understanding conceptually what this is doing.
> ASTTask should be a member function, I think.

Moved it into a meber named `publishDiagnostics`


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:693
+        std::lock_guard<std::mutex> Lock(Mutex);
+        // LatestPreamble might be the last reference to old preamble, do not
+        // trigger destructor while holding the lock.
----------------
sammccall wrote:
> Didn't we decide to make this available only after building the golden AST and publishing diagnostics?
as discussed offline, this is harmless since it is being executed on ASTWorker thread and makes new preamble available to code completion/signature help faster.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76725