[PATCH] D95057: [clangd] Allow configuration database to be specified in config.
Kadir Cetinkaya via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sun Jan 24 23:21:40 PST 2021
kadircet added a comment.
I believe we should log some warning at startup if user has provided `--compile-commands-dir`. Saying that "CDB search customizations through config is disabled". (only emitting the warning when we hit a config with CDB search customization would be nicer, but plumbing and making ConfigCompiler aware of such things sounds terrifying :/)
mostly LG, didn't get a change to look at the tests yet (will do so soon)
================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:267
+ llvm::Optional<Config::CDBSearchSpec> Spec;
+ if (**F.CompilationDatabase == "Ancestors") {
+ Spec.emplace();
----------------
i hope no one tries to put their CDBs in a directory named Ancestors/None :)))
(it is definitely better than breaking any project with a top-level directory named Ancestors/None)
================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:278
+ // Drop trailing slash to put the path in canonical form.
+ // Should makeAbsolute do this?
+ llvm::StringRef Rel = llvm::sys::path::relative_path(*Path);
----------------
+1 i think it should.
================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:617
+ Child->Parent = &Info;
+ // Keep walking, whether we inserted or not, if parent link is missing.
+ // (If it's present, parent links must be present up to the root, so stop)
----------------
missing a `Child = &Info;` ?
================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:650
+ DirValues[I]->Cache = DirCaches[I];
+ if (DirCaches[I] == ThisCache)
+ DirValues[I]->State = DirInfo::TargetCDB;
----------------
nit: `DirValues[I]->State = DirCaches[I] == ThisCache ? DirInfo::TargetCDB : DirInfo::Unknown;`, to reduce branching.
================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:680
+ // Walk up to the next parent.
+ return shouldInclude(SearchPath(Info->Parent, P.getInt()));
}
----------------
s/P.getInt()/1
as we'll bail out otherwise.
================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:717
+ case Config::CDBSearchSpec::Ancestors:
+ SearchPaths[I].setInt(/*Recursive*/ 1);
+ SearchPaths[I].setPointer(addParents(AllFiles[I]));
----------------
s/Recursive/Recursive=/
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95057/new/
https://reviews.llvm.org/D95057
More information about the cfe-commits
mailing list