kadircet added a comment.
this mostly LGTM. there are some changes to the existing behavior; like
discovery order and not looking for other plugins under `build/`, but they seem
like non-harmful changes to me.
my biggest question is, have we considered updating `CompilationDatabasePlugin`
interface to provide a `loadFromBuffer` and `relativeFileName` virtual methods.
I think loading logic could've been a lot simpler and more uniform that way.
i think we discussed this option but i don't remember the outcome, sorry :(.
================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:185
+ bool MayCache = load(*TFS.view(/*CWD=*/llvm::None));
+ if (MayCache) {
+ // Use new timestamp, as loading may be slow.
----------------
nit: inline `MayCache`
================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:205
+ auto Stat = FS.status(Path);
+ if (!Stat || !Stat->isRegularFile()) {
+ Size = NoFileCached;
----------------
what about symlinks ? i think users usually have `cc.json ->
obscure_build_dir/cc.json` as a symlink.
================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:224
+ if (HasOldData && NewContentHash == ContentHash)
+ // mtime changed but data is the same: avoid rebuilding the CDB.
+ return {LoadResult::FoundSameData, nullptr};
----------------
also update cached mtime ?
================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:267
+ // Wrapper for {Fixed,JSON}CompilationDatabase::loadFromBuffer.
+ std::unique_ptr<tooling::CompilationDatabase> (*Parser)(
+ PathRef,
----------------
nit: can we have a functor(probably llvm::function_ref) rather than a function
pointer?
================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:322
+ auto Plugin = Entry.instantiate();
+ if (auto CDB = Plugin->loadFromDirectory(Path, Error)) {
+ log("Loaded compilation database from {0} with plugin {1}", Path,
----------------
it is not crucial but we used to try to load these from `Path + "/build"` as
well
================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:456
CDBLookupResult Result) const {
+ vlog("Broadcasting compilation database from {0}", Result.PI.SourceRoot);
assert(Result.CDB && "Trying to broadcast an invalid CDB!");
----------------
why not `log` instead of `vlog`? this shouldn't be too noisy anyway.
================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.h:133
+
+ std::chrono::steady_clock::duration RevalidateAfter;
+ std::chrono::steady_clock::duration RevalidateMissingAfter;
----------------
why not directly store `Options` ?
================
Comment at:
clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp:393
+
+auto HasFlag(llvm::StringRef Flag) { return HasFlag(Flag, "dummy.cc"); }
+
----------------
nit: maybe a `MATCHER_P` instead ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92663/new/
https://reviews.llvm.org/D92663
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
