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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
