[PATCH] D64247: [clangd] Filter out non-governed files from broadcast

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:118
+      DirectoryHasCDB[Path] = false;
+      return false;
+    });
----------------
return Path == Result.PI.SourceRoot?


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:125
+    std::lock_guard<std::mutex> Lock(Mutex);
+    for (auto &Entry : DirectoryHasCDB) {
+      auto Res = getCDBInDirLocked(Entry.first());
----------------
(Why not do this in the one pass above? Doesn't seem worth two passes to avoid locking while iterating over AllFiles)


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:134
+  for (llvm::StringRef File : AllFiles) {
+    // This check assumes that a source file is governed by the closest
+    // compilation database. Independent of whether it has an entry for that
----------------
assumes isn't strong enough here - we're defining it this way, and should say why.

e.g. "A file is only part of this CDB if lookup for the file would find this CDB."


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:183
+
+  // FIXME: Maybe make the following part async, since this can block retrieval
+  // of compile commands.
----------------
I don't think we'll ever fix this, feel free to drop the FIXME if you like.


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.h:115
   /// is located.
   llvm::Optional<Path> CompileCommandsDir;
 };
----------------
(random note: we should really rename this something like FixedCDBDir. Probably not in this patch, but what a confusing name)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64247