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