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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev planned changes to this revision.
kbobyrev added a comment.

Also



================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:164
+
+  set(_GRPC_GRPCPP gRPC::grpc++)
+  set(_PROTOBUF_LIBPROTOBUF protobuf::libprotobuf)
----------------
sammccall wrote:
> Can we give these slightly less weird names :-)
Inlined everything except (newly) `GRPC_CPP_PLUGIN`.


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:169
+
+  add_definitions(-DGOOGLE_PROTOBUF_NO_RTTI=1 -DCLANGD_SHARED_INDEX)
+  include_directories(${Protobuf_INCLUDE_DIRS})
----------------
sammccall wrote:
> CLANGD_SHARED_INDEX unused?
Yep, this one was used within Dexp when I was trying to extend it in this patch, but I realized I would need to either hide a bunch of commands or support some of them which would require the full Index interface implementation, Symbol deep copy and passing it through gRPC, etc. I guess we'd need to use it at some point anyway since there will be at least _some_ path-specific code if we don't build with gRPC by default (not rational for keeping it in this patch, just a bit of context in case you're interested).

Good catch, will remove!


================
Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:262
 
 struct {
   const char *Name;
----------------
sammccall wrote:
> (unrelated formatting changes to this file)
This one is super weird, I even reset the file for the revision I was basing `arc diff` on, but it didn't work for some reason :(


================
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")
----------------
sammccall wrote:
> generate a library wrapping those instead, for dependents to link against?
Great idea, thanks!


================
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")
----------------
sammccall wrote:
> used once, inline? (and grpc_hdrs)
Why once? It's used at least 3 times at the moment - once in the generation command and in both targets. Maybe once I put all of it in the library that becomes twice? Let me see.


================
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}"
----------------
sammccall wrote:
> Needed? it doesn't import anything
I'm afraid it's needed, I tried to get by without it but `protoc` fails with an error:

```
File does not reside within any path specified using --proto_path (or -I).  You must specify a --proto_path which encompasses this file.  Note that the proto_path must be an
 exact prefix of the .proto file names -- protoc is too dumb to figure out when two paths (e.g. absolute and relative) are equivalent (it's harder than you think).
```


================
Comment at: clang-tools-extra/clangd/index/shared/SharedIndex.proto:15
+
+message LookupRequestProto { string Name = 1; }
+
----------------
sammccall wrote:
> nit: LookupRequest, no Proto suffix. (If we need namespacing, we should use a namespace)
Yep, thought this would be confusing :) We already have a bunch of `*Request` things but I didn't know `package <...>` would wrap it into the appropriate namespace. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77794