sammccall added inline comments.
================ Comment at: clangd/TUScheduler.cpp:286 } // unlock Mutex. RequestsCV.notify_one(); } ---------------- ilya-biryukov wrote: > Change to `notify_all()`? Otherwise we might wake up some thread waiting on > `blockUntilIdle()`, but not the worker thread. Done. In fact I had changed this to `notify_all`, and changed it back because it's safe: - this class isn't threadsafe - therefore there are only two interesting threads, the caller thread and the worker thread - each thread can only wake up the other one, so there's no unsatisfied waiter But as discussed offline, notify_all() is just a less surprising default, and will fail in more obvious ways when we write threading bugs. ================ Comment at: clangd/Threading.h:68 + Deadline D, Func F) { + std::unique_lock<std::mutex> Lock(Mutex); + if (D) ---------------- ilya-biryukov wrote: > 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. Yeah, fair enough. ================ Comment at: unittests/clangd/ClangdTests.cpp:783 public: - NoConcurrentAccessDiagConsumer(std::promise<void> StartSecondReparse) - : StartSecondReparse(std::move(StartSecondReparse)) {} + std::atomic<int> Count = {0}; ---------------- ilya-biryukov wrote: > Maybe use paren initializers? Looks a bit less curly :-) > ``` > std::atomic<int> Count(0); > ``` This is a member initializer, I think I have to use this syntax :-( 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