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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:94
+
+  // It doesn't make sense to perform a traversal if user said their compilation
+  // database is in a different place. Return that directory for all files.
----------------
The duplication here is a bit troublesome.

And we have divergences in behavior - probably not important ones, but I can't say I know for sure. (e.g. here we won't create the CDB as a side-effect of getPathInfo() if there's a CompileCommandsDir, and will return the dir even if the CDB doesn't exist)

I think we might want to have some central logic parameterized by a struct, e.g.
```
struct DirectoryBasedCompilationDatabase::Lookup {
  // inputs
  PathRef File;
  bool ShouldBroadcast;

  // outputs
  tooling::CompilationDatabase *CDB;
  ProjectInfo Info;
  bool EverBroadcast;
};
lookupCDB(Lookup); // replaces getCDBInDirLocked
```


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:176
+    std::vector<std::string> GovernedFiles;
+    for (llvm::StringRef File : CDB->getAllFiles()) {
+      auto PI = getProjectInfo(File);
----------------
here we're locking/unlocking the mutex maybe thousands of times, to do mostly redundant lookups into the cache

Seems worth doing at least one of:
 - deduplicate directories, so we lock once for each distinct directory and build a string->bool map. Then use that map to filter the results
 - lock around the whole loop and use getCDBInDirLocked()


================
Comment at: clang-tools-extra/clangd/index/Background.cpp:653
+    auto PI = CDB.getProjectInfo(File);
+    assert(PI && "Found CDB but no ProjectInfo!");
+
----------------
This looks like a bad assertion: it should be OK to provide compile commands but not project info.

(Otherwise getProjectInfo should be pure virtual, but I'm concerned about the raciness if we're going to insist they're consistent but not provide any way of synchronizing)

I'd suggest we should just not index such files (for now). Later we can start passing "" to the index storage factory to get the fallback storage (I think today we pass "", but the factory doesn't handle it - oops!)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64247