[PATCH] D94606: [clangd] Move DirBasedCDB broadcasting onto its own thread.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:496
+  std::condition_variable CV;
+  std::atomic<bool> ShouldStop = {false}; // Must notify CV after writing.
+  std::deque<CDBLookupResult> Queue;
----------------
nit: we already hold the lock within the loop while reading. it is nice that we no longer need to acquire the lock during destructor, but is it worth the extra mental load that comes with memory orderings? (also IIRC, default load/store behaviors are already acquire-release)


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:541
+      });
+      Queue.push_back(std::move(Task));
+    }
----------------
don't we need some context propagation here? previously broadcasting was running with the context of the astworker that discovered the cdb. i suppose it is not needed at the moment, as most of the stuff we do in ::process isn't context-aware, but I can't be sure if we don't have any users that make use of some context-aware FS.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94606