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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
