[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

[Sam McCall via Phabricator via cfe-commits]

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