ilya-biryukov added inline comments.

Comment at: clangd/TUScheduler.cpp:286
   } // unlock Mutex.
Change to `notify_all()`? Otherwise we might wake up some thread waiting on 
`blockUntilIdle()`, but not the worker thread.

Comment at: clangd/TUScheduler.cpp:314
+    }
+    RequestsCV.notify_one();
We should probably call `notify_all()` here.
Having multiple `blockUntilIdle` might not work otherwise.

Comment at: clangd/Threading.h:66
+template <typename Func>
+LLVM_NODISCARD bool wait(std::mutex &Mutex, std::condition_variable &CV,
+                         Deadline D, Func F) {
sammccall wrote:
> ilya-biryukov wrote:
> > Maybe move this helper to .cpp file?
> it's public, and it's a template - what am I missing?
Sorry, I missed that it's used outside Threading.h

Comment at: clangd/Threading.h:68
+                         Deadline D, Func F) {
+  std::unique_lock<std::mutex> Lock(Mutex);
+  if (D)
sammccall wrote:
> ilya-biryukov wrote:
> > Maybe keep the locking part out of this helper? It's often desirable to 
> > hold the lock after `wait()`. This will model how 
> > `std::condition_variable::wait` is defined. 
> I tried this, but find the API just way too grungy - we need to pass a 
> `unique_lock<std::mutex>&`?
> Generally I'd hope we're aiming for something a little higher level than the 
> standard library!
> We don't actually ever need to do anything with the lock held. If we did, 
> what about passing a lambda?
The higher-level abstraction would certainly be appealing to me as well and I 
would welcome it.
I don't think we get it here, though. It's actually a bit confusing that the 
function calls into STL's versions of wait and wait_until, has a similar name, 
but does a slightly different thing.

Comment at: unittests/clangd/ClangdTests.cpp:783
-    NoConcurrentAccessDiagConsumer(std::promise<void> StartSecondReparse)
-        : StartSecondReparse(std::move(StartSecondReparse)) {}
+    std::atomic<int> Count = {0};
Maybe use paren initializers?  Looks a bit less curly :-)
std::atomic<int> Count(0);

  rCTE Clang Tools Extra

cfe-commits mailing list

Reply via email to