[PATCH] D80784: [clangd][NFC] Explode ReceivedPreamble into a CV

[Sam McCall via Phabricator via cfe-commits]

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