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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D95057#2518903 <https://reviews.llvm.org/D95057#2518903>, @kadircet wrote:

> 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".

So I considered this, but it depends on what we want the eventual interaction of flags and config to be.

A) if we're going to eliminate any flags that overlap with config, the message should say the flag is deprecated
B) if config is going to override flags, the message should say that the flag overrides config for now but this will change
C) if flags are going to override config, I don't think we should log a message when any such flag is used...

Personally I lean toward C and so don't want to log. I realize that there's legacy users of this flag that may not know the alternative, but equally there are future users of this flag that will be not-helped by such a log message.



================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:267
+      llvm::Optional<Config::CDBSearchSpec> Spec;
+      if (**F.CompilationDatabase == "Ancestors") {
+        Spec.emplace();
----------------
kadircet wrote:
> 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)
Haha, yes... you could always write ./Ancestors then, I suppose.

(Obviously we can work around this with a more complicated structure... but I'm not sure it's a good trade)


================
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);
----------------
kadircet wrote:
> +1 i think it should.
Right... the reason I didn't make the change in this patch is that it affected MountPoint of indexes, and there were tests of that, and code using starts_with in a way that suggested it might be important.

So we should clean this up somehow, but I didn't want to bite it off here.


================
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)
----------------
kadircet wrote:
> missing a `Child = &Info;` ?
Nice catch, I guess I didn't have enough levels in my tests...


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:650
+      DirValues[I]->Cache = DirCaches[I];
+      if (DirCaches[I] == ThisCache)
+        DirValues[I]->State = DirInfo::TargetCDB;
----------------
kadircet wrote:
> nit: `DirValues[I]->State = DirCaches[I] == ThisCache ? DirInfo::TargetCDB : DirInfo::Unknown;`, to reduce branching.
This seems less clear to me, the branch is fairly predictable, and this function is pretty cold - I'd rather keep init as is for readability.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95057/new/

https://reviews.llvm.org/D95057