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

[Ilya Biryukov via Phabricator via cfe-commits]

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