This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Allow configuration database to be specified in config.
ClosedPublic

Authored by sammccall on Jan 20 2021, 9:35 AM.

Details

Summary

This allows more flexibility than --compile-commands-dir flag or ancestor-based discovery.

See https://github.com/clangd/clangd/issues/116

Diff Detail

Event Timeline

sammccall created this revision.Jan 20 2021, 9:35 AM
sammccall requested review of this revision.Jan 20 2021, 9:35 AM
sammccall updated this revision to Diff 318499.Jan 22 2021, 5:44 AM

Rebase, implement yaml bits (no tests yet)

sammccall updated this revision to Diff 318526.Jan 22 2021, 7:37 AM

Add tests and fix bugs

sammccall retitled this revision from [clangd] WIP: configurable compilation database directory to [clangd] Allow configuration database to be specified in config..Jan 22 2021, 7:38 AM
sammccall edited the summary of this revision. (Show Details)
sammccall added a reviewer: kadircet.
nridge added a subscriber: nridge.Jan 24 2021, 12:21 AM

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)

clang-tools-extra/clangd/ConfigCompile.cpp
267

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)

278

+1 i think it should.

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
617

missing a Child = &Info; ?

650

nit: DirValues[I]->State = DirCaches[I] == ThisCache ? DirInfo::TargetCDB : DirInfo::Unknown;, to reduce branching.

680

s/P.getInt()/1

as we'll bail out otherwise.

717

s/Recursive/Recursive=/

kadircet added inline comments.Jan 25 2021, 12:00 AM
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
140

maybe also try with an absolute path in CompilationDatabase ?

sammccall updated this revision to Diff 318980.Jan 25 2021, 5:56 AM
sammccall marked 4 inline comments as done.

address comments

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.

clang-tools-extra/clangd/ConfigCompile.cpp
267

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)

278

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.

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
617

Nice catch, I guess I didn't have enough levels in my tests...

650

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.

kadircet accepted this revision.Jan 25 2021, 6:43 AM

thanks, lgtm!

clang-tools-extra/clangd/ConfigCompile.cpp
278

SG. As a note to future self, I don't remember the tests but in theory the startswith should be fine with and without the trailing slash.

This revision is now accepted and ready to land.Jan 25 2021, 6:43 AM