[PATCH] D98404: [clangd] Optionally add reflection for clangd-index-server

Mon Mar 15 02:40:59 PDT 2021

kadircet added inline comments.

================
Comment at: clang-tools-extra/clangd/index/remote/server/CMakeLists.txt:12

+if (ENABLE_GRPC_REFLECTION)
+  set(REFLECTION_LIBRARY grpc++_reflection)
----------------
kadircet wrote:
> kbobyrev wrote:
> > kadircet wrote:
> > > can we move this piece into FindGRPC.cmake instead?
> > 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` :(
> 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`
this is not needed anymore right ?

================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:38

+#ifdef ENABLE_GRPC_REFLECTION
+#include <grpc++/ext/proto_server_reflection_plugin.h>
----------------
this should be `#if` rather than an `#ifdef`, same below.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98404/new/

https://reviews.llvm.org/D98404