kbobyrev added inline comments.
================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:156
+
+option(GRPC_INSTALL_PATH "Path to gRPC library installation." OFF)
+if (GRPC_INSTALL_PATH)
----------------
sammccall wrote:
> sammccall wrote:
> > sammccall wrote:
> > > We'll eventually want to move all this out of clangd I guess? Maybe add
> > > FIXMEs for that?
> > this should be controlled by a top-level LLVM_ENABLE_GRPC or LLVM_USE_GRPC
> > or so flag (not sure what the convention is).
> > Using GRPC_INSTALL_PATH is tempting now but there are multiple ways to
> > configure it so it won't scale.
> > And at least in some cases we should be able to autodetect it, so we'll
> > need a dedicated enable flag.
> GRPC_INSTALL_PATH should be set() instead of option().
Do you mean moving gRPC library setup logic to `llvm/CMakeLists.txt`?
================
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}"
----------------
sammccall wrote:
> can we wrap this in a function?
Are you proposing making a function (e.g. `generate_protos`) that would emit
required source files? I'd have to do some manual dependency management in this
case, that's why I did `add_custom_command` which seems to be the idiomatic way
of generating targets with the right dependencies and properties (being
generated files).
Could you please explain the benefit of wrapping this into a function?
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
