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