[PATCH] D129100: [clangd] Support external throttler for preamble builds

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks, lgtm!



================
Comment at: clang-tools-extra/clangd/ClangdServer.h:108
+    /// This throttler controls which preambles may be built at a given time.
+    clangd::PreambleThrottler *PreambleThrottler = nullptr;
+
----------------
why the qualification ?


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:414
+private:
+  unsigned ID;
+  PreambleThrottler *Throttler;
----------------
use PreambleThrottler::RequestID instead of unsigned?


================
Comment at: clang-tools-extra/clangd/TUScheduler.h:90
 
+// PreambleThrottler controls which preambles can build at any given time.
+// This can be used to limit overall concurrency, and to prioritize some
----------------
triple slashes here and inside the class


================
Comment at: clang-tools-extra/clangd/TUScheduler.h:106
+  // Does not block, may eventually invoke the callback to satisfy the request.
+  // If cancel() is called, the callback will not be invoked afterwards.
+  // If the callback is invoked, release() must be called afterwards.
----------------
there's no cancel anymore.


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1499
+
+    S.remove(Filenames.back());
+    // Now shut down the TU Scheduler.
----------------
sequencing is hard here but it'd be nice to ensure release is actually seen.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129100/new/

https://reviews.llvm.org/D129100