[PATCH] D76304: [clangd] Update TUStatus api to accommodate preamble thread

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:277
       build(std::move(*CurrentReq));
+      bool IsEmpty = false;
+      {
----------------
sammccall wrote:
> kadircet wrote:
> > sammccall wrote:
> > > Seems clearer to do this immediately before blocking?
> > > 
> > > at the top:
> > > 
> > > ```
> > > if (!NextReq) {
> > >   Lock.unlock();
> > >   StatusManager.setPreambleAction(Idle);
> > >   Lock.lock();
> > >   ReqCV.wait(NextReq || Done);
> > > }
> > > if (Done)
> > >   break;
> > > ```
> > i agree, but I wanted to keep the current semantics. we only start emitting tustatuses after thread starts executing the first request.
> > 
> > happy to change *both* preamblethread and astworker to emit before blocking though. wdyt?
> I think the difference is moot - we never create either the AST or preamble worker until there's something to do.
> 
> The code around scheduling/sleeping in the AST worker thread is way more complicated, and I'm not confident moving the status broadcast to the top of the loop would be clearer there.
> 
> Up to you: if you think both are clearer, move both. If you think the preamble is clearer at the top and AST worker at the bottom, then you can choose between consistency and clarity :-)
as discussed offline, this might make tests flaky due to a possible race between this thread and the thread issuing the write. so leaving it as it is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76304