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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov 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>;
----------------
Maybe remove the comment or add more context (i.e. add references) on why the overflow is buggy?


================
Comment at: clangd/Threading.h:66
+template <typename Func>
+LLVM_NODISCARD bool wait(std::mutex &Mutex, std::condition_variable &CV,
+                         Deadline D, Func F) {
----------------
Maybe move this helper to .cpp file?


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


================
Comment at: clangd/Threading.h:83
 
-  void waitForAll();
+  bool waitForAll(Deadline D = llvm::None) const;
   void runAsync(UniqueFunction<void()> Action);
----------------
Add `LLVM_NODICARD` here?
For that particular method maybe we could have two overload: with and without the deadline, i.e.
```
  void waitForAll() const;
  LLVM_NODISCARD bool waitForAll(Deadline D) const;
```

There are places (like the destructor of this class) where the first overload is used and consuming the return value is just adding noise, but clients passing the deadline (e.g. `blockUntilIdle()`) should definitely consume the return value.


================
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};
 
----------------
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43127