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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
