[PATCH] D43648: [clangd] Debounce streams of updates.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/TUScheduler.h:56
+              std::chrono::steady_clock::duration UpdateDebounce =
+                  std::chrono::milliseconds(500));
   ~TUScheduler();
----------------
Maybe we could remove this default argument now that `ClangdServer` also has the default debounce?
I guess the key thing that bothers me is having multiple places that have the magical default constant of `500ms`, it would be nice to have it in one place.


================
Comment at: clangd/Threading.h:76
+private:
+  enum Type { Zero, Infinite, Finite } Type;
+  Deadline(enum Type Type) : Type(Type) {}
----------------
Maybe prefer grouping all the fields together? It seems we're giving that up to avoid writing one extra instance of `enum Type`.

```
  enum Type { Zero, Infinite, Finite };
  Deadline(enum Type Type) : Type(Type) {}

  enum Type Type;
  std::chrono::steady_clock::time_point Time;
```


================
Comment at: clangd/Threading.h:83
 Deadline timeoutSeconds(llvm::Optional<double> Seconds);
+/// Wait once for on CV for the specified duration.
+void wait(std::unique_lock<std::mutex> &Lock, std::condition_variable &CV,
----------------
s/Wait for on/Wait on/?


================
Comment at: unittests/clangd/TUSchedulerTests.cpp:127
+                  /*ASTParsedCallback=*/nullptr,
+                  /*UpdateDebounce=*/std::chrono::milliseconds(50));
+    auto Path = testPath("foo.cpp");
----------------
I wonder if the default debounce of `500ms` will make other tests (especially those that use `ClangdServer`) too slow?
Maybe we should consider settings a smaller default (maybe even `Deadline::zero()`?) and having `500ms` set only by `ClangdLSPServer`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43648