[PATCH] D77794: [clangd] Pull installed gRPC and introduce shared-index-(server|client)

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Naming throughout - sorry to be a pain. I think "shared index" is less precise than it could be. WDYT about "Remote index"?



================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:156
+
+option(GRPC_INSTALL_PATH "Path to gRPC library installation." OFF)
+if (GRPC_INSTALL_PATH)
----------------
We'll eventually want to move all this out of clangd I guess? Maybe add FIXMEs for that?


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:164
+
+  set(_GRPC_GRPCPP gRPC::grpc++)
+  set(_PROTOBUF_LIBPROTOBUF protobuf::libprotobuf)
----------------
Can we give these slightly less weird names :-)


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:169
+
+  add_definitions(-DGOOGLE_PROTOBUF_NO_RTTI=1 -DCLANGD_SHARED_INDEX)
+  include_directories(${Protobuf_INCLUDE_DIRS})
----------------
CLANGD_SHARED_INDEX unused?


================
Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:262
 
 struct {
   const char *Name;
----------------
(unrelated formatting changes to this file)


================
Comment at: clang-tools-extra/clangd/index/shared/CMakeLists.txt:4
+
+set(SharedIndex_proto_srcs "${CMAKE_CURRENT_BINARY_DIR}/SharedIndex.pb.cc")
+set(SharedIndex_proto_hdrs "${CMAKE_CURRENT_BINARY_DIR}/SharedIndex.pb.h")
----------------
generate a library wrapping those instead, for dependents to link against?


================
Comment at: clang-tools-extra/clangd/index/shared/CMakeLists.txt:5
+set(SharedIndex_proto_srcs "${CMAKE_CURRENT_BINARY_DIR}/SharedIndex.pb.cc")
+set(SharedIndex_proto_hdrs "${CMAKE_CURRENT_BINARY_DIR}/SharedIndex.pb.h")
+set(SharedIndex_grpc_srcs "${CMAKE_CURRENT_BINARY_DIR}/SharedIndex.grpc.pb.cc")
----------------
used once, inline? (and grpc_hdrs)


================
Comment at: clang-tools-extra/clangd/index/shared/CMakeLists.txt:8
+set(SharedIndex_grpc_hdrs "${CMAKE_CURRENT_BINARY_DIR}/SharedIndex.grpc.pb.h")
+add_custom_command(
+      OUTPUT "${SharedIndex_proto_srcs}" "${SharedIndex_proto_hdrs}" "${SharedIndex_grpc_srcs}" "${SharedIndex_grpc_hdrs}"
----------------
can we wrap this in a function?


================
Comment at: clang-tools-extra/clangd/index/shared/CMakeLists.txt:13
+        --cpp_out "${CMAKE_CURRENT_BINARY_DIR}"
+        -I "${SharedIndexProtoPath}"
+        --plugin=protoc-gen-grpc="${_GRPC_CPP_PLUGIN_EXECUTABLE}"
----------------
Needed? it doesn't import anything


================
Comment at: clang-tools-extra/clangd/index/shared/SharedIndex.proto:1
+//===--- Completion.proto - Shared index code completion protobuf ---------===//
+//
----------------
Please drop "Shared" from the various filenames, we're already in a directory called Shared.

nit: header has the wrong name


================
Comment at: clang-tools-extra/clangd/index/shared/SharedIndex.proto:10
+syntax = "proto3";
+
+service SharedIndex {
----------------
missing package (namespace)


================
Comment at: clang-tools-extra/clangd/index/shared/SharedIndex.proto:15
+
+message LookupRequestProto { string Name = 1; }
+
----------------
nit: LookupRequest, no Proto suffix. (If we need namespacing, we should use a namespace)


================
Comment at: clang-tools-extra/clangd/index/shared/SharedIndex.proto:17
+
+message LookupReply { string SymbolYAML = 1; }
----------------
symbol_yaml.
This is the style the codegen expects, and fighting it yields ugly results.


================
Comment at: llvm/cmake/modules/LLVMProcessSources.cmake:112
           endif()
-          message(SEND_ERROR "Found unknown source file ${fn_relative}
-Please update ${CMAKE_CURRENT_LIST_FILE}\n")
+#           message(SEND_ERROR "Found unknown source file ${fn_relative}
+# Please update ${CMAKE_CURRENT_LIST_FILE}\n")
----------------
we can't do this :-)

need to move each tool to its own directory instead, which is annoying.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77794