ilya-biryukov added inline comments.
================ Comment at: clangd/TUScheduler.cpp:286 } // unlock Mutex. RequestsCV.notify_one(); } ---------------- 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 public: - 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); ``` Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43127 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits