[PATCH] D92381: [clangd] Extract per-dir CDB cache to its own threadsafe class. NFC

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:91
+  // Whether a new CDB has been loaded but not broadcast yet.
+  bool Dirty = false;
+  // Last loaded CDB, meaningful if CachePopulated is set.
----------------
maybe rename this to `DidBroadcast` if we are not planning to add extra meaning to `Dirty` in the near future?


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:119
+    if (CachePopulated) {
+      ShouldBroadcast = Dirty;
+      Dirty = false;
----------------
what if caller is not willing to broadcast?


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:253
+    for (DirectoryCache *Candidate : getDirectoryCaches(SearchDirs)) {
+      if ((CDB = Candidate->get(ShouldBroadcast))) {
+        DirCache = Candidate;
----------------
won't this overwrite `ShouldBroadcast` ?

for example if we get a query with `ShouldBroadcast` set to `true`, but first searchdir doesn't have a CDB underneath, for all the consecutive calls this will be `false`.


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:263
 
-    Result.CDB = Entry->CDB.get();
-    Result.PI.SourceRoot = Entry->Path;
-  }
+  // Mark CDB as broadcasted to make sure discovery is performed once.
+  CDBLookupResult Result;
----------------
stale comment.


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.h:88
+  mutable llvm::StringMap<DirectoryCache> DirCaches;
+  mutable std::mutex Mutex;
+  mutable std::unique_ptr<DirectoryCache> OnlyDirCache;
----------------
it is kind of hard to figure out if this mutex is also meant for OnlyDirCache. maybe separate it with a line or put some explicit `governed by` annotations?


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.h:89
+  mutable std::mutex Mutex;
+  mutable std::unique_ptr<DirectoryCache> OnlyDirCache;
+
----------------
some comments about semantics of `OnlyDirCache` ? I suppose existing of it implies `DirCaches` won't be used at all ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92381