[PATCH] D95057: [clangd] Allow configuration database to be specified in config.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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