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