This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Optionally add reflection for clangd-index-server
ClosedPublic

Authored by kbobyrev on Mar 11 2021, 2:45 AM.

Diff Detail

Event Timeline

kbobyrev created this revision.Mar 11 2021, 2:45 AM
kbobyrev requested review of this revision.Mar 11 2021, 2:45 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 11 2021, 2:45 AM
kadircet added inline comments.Mar 11 2021, 3:10 AM
clang-tools-extra/clangd/index/remote/server/CMakeLists.txt
12

can we move this piece into FindGRPC.cmake instead?

llvm/cmake/modules/FindGRPC.cmake
21

nit: is this change needed?

kbobyrev updated this revision to Diff 329913.Mar 11 2021, 4:00 AM
kbobyrev marked 2 inline comments as done.

Address review comments.

clang-tools-extra/clangd/index/remote/server/CMakeLists.txt
12

Yes, but I think only the set(REFLECTION_LIBRARY grpc++_reflection) part? Having the -DENABLE_GRPC_REFLECTION definition globally is probably not very nice and as with the other target under clangd/index/remote, add_target_definition doesn't work because of add_clan_executable :(

kadircet added inline comments.Mar 11 2021, 7:09 AM
clang-tools-extra/clangd/index/remote/server/CMakeLists.txt
12

ah i forgot about that one, can we have this in clang-tools-extra/clangd/Features.inc.in then, with rest of our definitions?

you might also want to canonicalize the option via llvm_canonicalize_cmake_booleans

kbobyrev updated this revision to Diff 330573.Mar 15 2021, 1:26 AM
kbobyrev marked an inline comment as done.

Add the variable to Features.inc.in and canonicalize.

Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2021, 1:26 AM
kadircet added inline comments.Mar 15 2021, 2:40 AM
clang-tools-extra/clangd/index/remote/server/CMakeLists.txt
12

this is not needed anymore right ?

clang-tools-extra/clangd/index/remote/server/Server.cpp
38

this should be #if rather than an #ifdef, same below.

kbobyrev updated this revision to Diff 330589.Mar 15 2021, 3:03 AM
kbobyrev marked 2 inline comments as done.

Resolve review comments.

kbobyrev updated this revision to Diff 330592.Mar 15 2021, 3:17 AM

Include Features.inc in Server.cpp.

kbobyrev updated this revision to Diff 330675.Mar 15 2021, 8:37 AM

Add explicit CMake option.

kadircet accepted this revision.Mar 15 2021, 11:44 AM

thanks, lgtm!

clang-tools-extra/clangd/CMakeLists.txt
190 ↗(On Diff #330675)

i think this option belongs to FindGRPC.cmake, as it configures the grpc linking behavior in general, not something specific to clangd.

This revision is now accepted and ready to land.Mar 15 2021, 11:44 AM
kbobyrev updated this revision to Diff 330772.Mar 15 2021, 12:59 PM
kbobyrev marked an inline comment as done.

Resolve post-LGTM comments.

This revision was landed with ongoing or failed builds.Mar 15 2021, 1:07 PM
This revision was automatically updated to reflect the committed changes.