This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Enable auto-index behind a flag.
ClosedPublic

Authored by sammccall on Nov 26 2018, 6:08 AM.

Details

Summary

Ownership and configuration:
The auto-index (background index) is maintained by ClangdServer, like Dynamic.
(This means ClangdServer will be able to enqueue preamble indexing in future).
For now it's enabled by a simple boolean flag in ClangdServer::Options, but
we probably want to eventually allow injecting the storage strategy.

New 'sync' command:
In order to meaningfully test the integration (not just unit-test components)
we need a way for tests to ensure the asynchronous index reads/writes occur
before a certain point.
Because these tests and assertions are few, I think exposing an explicit "sync"
command for use in tests is simpler than allowing threading to be completely
disabled in the background index (as we do for TUScheduler).

Bugs:
I fixed a couple of trivial bugs I found while testing, but there's one I can't.
JSONCompilationDatabase::getAllFiles() may return relative paths, and currently
we trigger an assertion that assumes they are absolute.
There's no efficient way to resolve them (you have to retrieve the corresponding
command and then resolve against its directory property). In general I think
this behavior is broken and we should fix it in JSONCompilationDatabase and
require CompilationDatabase::getAllFiles() to be absolute.

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Nov 26 2018, 6:08 AM

lgtm (will let @kadircet stamp )

test/clangd/background-index.test
19 ↗(On Diff #175237)

Maybe make this a FIXME?

kadircet accepted this revision.Nov 26 2018, 6:43 AM

Sorry for the bugs, thanks for the fixes :D

clangd/ClangdServer.cpp
515 ↗(On Diff #175237)

Should we change BackgroundIndexs signature to indicate success failure? Not sure it can block but just in case something weird happens.

test/clangd/background-index.test
13 ↗(On Diff #175237)

shouldn't this one be running ?

16 ↗(On Diff #175237)

and this

This revision is now accepted and ready to land.Nov 26 2018, 6:43 AM
kadircet added inline comments.Nov 26 2018, 6:55 AM
test/clangd/background-index.test
16 ↗(On Diff #175237)

also I suppose we might wanna delete this file after test ends(if the temp directory is not already deleted at exit)

kadircet added inline comments.Nov 26 2018, 7:00 AM
test/clangd/background-index.test
13 ↗(On Diff #175237)

Argh, sorry somehow I got confused and thought these were commented-out.. NVM

sammccall updated this revision to Diff 175249.Nov 26 2018, 7:04 AM
sammccall marked an inline comment as done.

Make blockUntilIdleForTest() accept a timeout, update comment.

sammccall marked an inline comment as done.Nov 26 2018, 7:39 AM
sammccall added inline comments.
test/clangd/background-index.test
16 ↗(On Diff #175237)

Convention is that tests are responsible for cleaning up the temp directory *before* they run, but not after.

This revision was automatically updated to reflect the committed changes.