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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/Threading.h:60
+/// A point in time we may wait for, or None to wait forever.
+/// (We use Optional because buggy implementations of std::chrono overflow...)
+using Deadline = llvm::Optional<std::chrono::steady_clock::time_point>;
----------------
ilya-biryukov wrote:
> Maybe remove the comment or add more context (i.e. add references) on why the overflow is buggy?
Done. (Making specific reference to time_point::max() + overflow probably makes it clear why it's unusable?)


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


================
Comment at: clangd/Threading.h:68
+                         Deadline D, Func F) {
+  std::unique_lock<std::mutex> Lock(Mutex);
+  if (D)
----------------
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?


================
Comment at: unittests/clangd/ClangdTests.cpp:784
   class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
-  public:
-    NoConcurrentAccessDiagConsumer(std::promise<void> StartSecondReparse)
-        : StartSecondReparse(std::move(StartSecondReparse)) {}
-
-    void onDiagnosticsReady(
-        PathRef File,
-        Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {
+    std::atomic<bool> InCallback = {false};
 
----------------
ilya-biryukov wrote:
> Do we need to change this test?
> It was specifically designed to keep the second request from overriding the first one before it was processed.
You're right. It wasn't obvious to me that this was what it was doing or why, so it seemed easy to simplify.
Added some comments and a sanity check assertion, reverted the logic changes.

(The change might make sense anyway - we rely on sleep already, a second sleep would get rid of all the synchronization without adding latency or changing behavior, but not in this patch)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43127