sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:158
+if (LLVM_USE_GRPC)
+ set(protobuf_MODULE_COMPATIBLE TRUE)
+ find_package(Protobuf CONFIG REQUIRED HINTS ${GRPC_INSTALL_PATH})
----------------
can we add a comment to the top of this section describing the setup that it
expects? (GRPC built from source and installed into ${GRPC_INSTALL_PATH})
================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:164
+
+ set_target_properties(protobuf::libprotobuf PROPERTIES IMPORTED_GLOBAL TRUE)
+ add_library(protobuf ALIAS protobuf::libprotobuf)
----------------
Comment: "grpc cmakelists gives the libraries slightly odd names, make them
match the conventional system-installed names"
================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:170
+
+ # NOTE: Callers of the function should also add CMake source directory to
+ # include path.
----------------
Add primary function comment before the note.
this comment is a bit confusing:
- it says callers of the function, when it means users of the proto
- it says source directory, I think it means binary directory
- it doesn't say why
I'd suggest:
Proto headers are generated in ${CMAKE_CURRENT_BINARY_DIR}.
Libraries that use these headers should adjust the include path.
================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:178
+ set(GeneratedProtoHeader "${CMAKE_CURRENT_BINARY_DIR}/Index.pb.h")
+ set(GeneratedGRPCSource "${CMAKE_CURRENT_BINARY_DIR}/Index.grpc.pb.cc")
+ set(GeneratedGRPCHeader "${CMAKE_CURRENT_BINARY_DIR}/Index.grpc.pb.h")
----------------
this should be optional, with a parameter to generate_protos (along with the
plugin and grpc_out args) - AIUI it's required for protos that define services
only.
Conventional name for this param is HasServices
================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:182
+ OUTPUT "${GeneratedProtoSource}" "${GeneratedProtoHeader}"
"${GeneratedGRPCSource}" "${GeneratedGRPCHeader}"
+ COMMAND ${GRPC_INSTALL_PATH}/bin/protoc
+ ARGS --grpc_out="${CMAKE_CURRENT_BINARY_DIR}"
----------------
I believe this should be Protobuf_DIR or so (find_package output) to avoid
getting confused if find_package found it somewhere else
================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:191
+ add_library(${LibraryName} ${GeneratedProtoSource} ${GeneratedProtoHeader}
${GeneratedGRPCSource} ${GeneratedGRPCHeader})
+ target_link_libraries(${LibraryName} gRPC::grpc++ protobuf::libprotobuf)
+ endfunction()
----------------
nit: grpc++ protobuf
================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:194
+
+ add_definitions(-DGOOGLE_PROTOBUF_NO_RTTI=1)
+ include_directories(${Protobuf_INCLUDE_DIRS})
----------------
these global side-effects are pretty scary, can we limit them somehow?
================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:157
+# FIXME(kirillbobyrev): Move the gRPC setup logic to llvm/CMakeLists.txt
+if (LLVM_USE_GRPC)
+ set(protobuf_MODULE_COMPATIBLE TRUE)
----------------
Having thought about this more - I think this flag should (at least for now) be
called something like `CLANGD_ENABLE_REMOTE` and documented as requiring grpc.
================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:158
+if (LLVM_USE_GRPC)
+ set(protobuf_MODULE_COMPATIBLE TRUE)
+ find_package(Protobuf CONFIG REQUIRED HINTS ${GRPC_INSTALL_PATH})
----------------
I think this code should be moved to `llvm/cmake/modules/FindGRPC.cmake` and
included here via `include(FindGRPC)`
================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:11
+
+package remote;
+
----------------
package clang.clangd.remote, to get namespaces compatible with the c++ code
================
Comment at: clang-tools-extra/clangd/index/remote/server/CMakeLists.txt:12
+
+ ${PROTOBUF_LIBRARY}
+ ${GRPC_LIBRARY}
----------------
protobuf and grpc++?
================
Comment at: llvm/CMakeLists.txt:1112
+# FIXME(kirillbobyrev): Document this in the LLVM docs.
+option(LLVM_USE_GRPC "Use gRPC library to enable remote index support for
Clangd" OFF)
+# FIXME(kirillbobyrev): Check if it works with optimized tablegen CMake option.
----------------
Sorry my thought before was incomplete - this should probably be
CLANGD_USE_GRPC for now with a fixme to move up to LLVM. Especially since all
the cmake code for this is in clangd for now.
================
Comment at: llvm/CMakeLists.txt:1114
+# FIXME(kirillbobyrev): Check if it works with optimized tablegen CMake option.
+set(GRPC_INSTALL_PATH "/usr" CACHE PATH "Path to gRPC library installation.")
----------------
This doesn't make sense - it's not a universally-usable directory, and there's
almost no chance that the cmake files are in /usr/lib/cmake/... since the
packages we've found don't include them.
I'd suggest for now we always require this to be set if using an
installed-grpc-with-cmakelists setup, and work out how to relax later.
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
