sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:661
+ std::unique_lock<std::mutex> Lock(Mutex);
+ RequestsCV.wait(Lock, [this] {
+ // Block until we reiceve a preamble request, unless a preamble already
----------------
kadircet wrote:
> sammccall wrote:
> > Does LatestPreamble signal RequestsCV or just PreambleCV?
> >
> > Seems like it might be less error-prone to have just one CV, signalled when
> > preamble requests are scheduled, latest preamble becomes available, and on
> > shutdown. The spurious wakeups shouldn't be a real problem, right?
> > Does LatestPreamble signal RequestsCV or just PreambleCV?
>
> it only signals `PreambleCV`. But it makes no sense for this code path to
> block on it, as it is signalled by the same thread. So in theory we should
> see `LatestPreamble` and not start blocking in such case.
>
> > Seems like it might be less error-prone to have just one CV, signalled when
> > preamble requests are scheduled, latest preamble becomes available, and on
> > shutdown. The spurious wakeups shouldn't be a real problem, right?
>
> Yeah spurious wake-ups aren't really a problem. I thought having separate CVs
> sounded more clear, as predicates would still look the same even if we only
> had one CV. So it would enforce us to signal/wait on the right condition
> variable.
>
> Happy to have only a single one too, do you have any suggestions for its name?
Yeah, I think that having the CV not cover all the conditions, with correctness
due to what threads the conditions are set on, is a bit too subtle.
I'd prefer a single CV named `PreambleCV` with an appropriate comment seems
clear, WDYT?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80784/new/
https://reviews.llvm.org/D80784
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
