This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add option to use remote index as static index
ClosedPublic

Authored by kbobyrev on Jul 14 2020, 2:45 PM.

Diff Detail

Event Timeline

kbobyrev created this revision.Jul 14 2020, 2:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2020, 2:45 PM
kbobyrev updated this revision to Diff 277985.Jul 14 2020, 2:47 PM

Remove unintended change.

kbobyrev planned changes to this revision.Jul 14 2020, 2:48 PM

Just stash changes, this is not a patch that is ready for a review.

kbobyrev updated this revision to Diff 278012.Jul 14 2020, 4:00 PM

Don't touch Marshalling.cpp

kbobyrev updated this revision to Diff 278013.Jul 14 2020, 4:02 PM

Remove remaining changes from Marshalling.cpp

kbobyrev planned changes to this revision.Jul 14 2020, 4:02 PM

Still WIP, stashing changes.

kbobyrev updated this revision to Diff 279457.Jul 21 2020, 2:29 AM

Add flag description.

kbobyrev updated this revision to Diff 279472.Jul 21 2020, 3:35 AM

Rebase on top of master.

kbobyrev retitled this revision from [clangd] Add remote index support for Clangd itself to [clangd] Add option to use remote index as static index.Jul 21 2020, 3:35 AM
kbobyrev added a reviewer: sammccall.
kbobyrev edited reviewers, added: hokein; removed: sammccall.Jul 24 2020, 6:37 AM
hokein added inline comments.Jul 24 2020, 7:40 AM
clang-tools-extra/clangd/tool/ClangdMain.cpp
457

not sure whether we need a new category, probably can live in Features?

459

since we hide this in the CLANGD_ENABLE_REMOTE flag, maybe remove the Hidden?

463

maybe ProjectRoot?

702

the new code section here should be guarded under #ifdef CLANGD_ENABLE_REMOTE

707

if remote-index is on, should we turn off the background index? I think this is our plan, right?

709

nit: use elog.

711

instead of exiting clangd, maybe just continue (since this is not a fatal error), and adjust the error message stating that index-file option is ignored.

kbobyrev updated this revision to Diff 280589.Jul 24 2020, 2:40 PM
kbobyrev marked 7 inline comments as done.

Address review comments.

kbobyrev updated this revision to Diff 280593.Jul 24 2020, 2:43 PM

Remove unused Remote category.

mostly good, just a concern about the linking dependence.

clang-tools-extra/clangd/CMakeLists.txt
138

the dependence here seems to be a layer violation. we add remote-index library when linking the clangDaemon, but clangdRemoteIndex *depends* on clangDaemon

I think we should do this when linking the clangd binary (in clangd/tool/CMakeLists.txt).

clang-tools-extra/clangd/tool/ClangdMain.cpp
463

nit: project-path -> project-root.

kbobyrev updated this revision to Diff 280822.Jul 27 2020, 1:53 AM
kbobyrev marked 2 inline comments as done.

Resolve more comments.

hokein accepted this revision.Jul 27 2020, 2:20 AM

looks good, thanks.

This revision is now accepted and ready to land.Jul 27 2020, 2:20 AM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Jul 27 2020, 4:40 AM
hans added inline comments.
clang-tools-extra/clangd/Features.inc.in
2

I'm guessing this should be @CLANGD_ENABLE_REMOTE@
The gn build was upset: http://45.33.8.238/linux/23942/step_4.txt
Speculatively fixed in 47a0254229ca425aa4e169c2db14e92b8db86784. Please shout if this was wrong.

hans added inline comments.Jul 27 2020, 5:05 AM
clang-tools-extra/clangd/tool/ClangdMain.cpp
702

I think it should be an #if since the macro is always defined (0 or 1). Doing that in 40d11a878044711708fb6738e4b78a4c9ac3de7b

thakis added a subscriber: thakis.Jul 27 2020, 9:34 AM
thakis added inline comments.
clang-tools-extra/clangd/Features.inc.in
2

Also, you probably either want #cmakedefine or you want to check with #if instead of #ifdef below. As-is, in the disabled case this will write #define CLANGD_ENABLE_REMOTE 0 in the disabled case, which causes the #ifdef to activate.