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

Wed Apr 15 16:01:52 PDT 2020

sammccall added a comment.

Thanks, this look really close now. A few things still in the wrong place, and naming/wording nits.

================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:156
+
+# This setup requires gRPC to be built from sources using CMake and installed to
+# ${GRPC_INSTALL_PATH} via -DCMAKE_INSTALL_PREFIX=${GRPC_INSTALL_PATH}.
----------------
This comment belongs in the relevant section of FindGRPC, not here

================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:162
+
+add_subdirectory(index/dex/dexp)
----------------
revert this move?

================
Comment at: clang-tools-extra/clangd/index/remote/CMakeLists.txt:7
+set(LLVM_LINK_COMPONENTS
+  LineEditor
+  Support
----------------
inline these into the client/server CMakeLists?

================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:14
+service Index {
+  rpc requestLookup(LookupRequest) returns (stream LookupReply) {}
+}
----------------
nit: `Lookup`, without the `request` prefix
UpperCamelCase is conventional for protobuf functions, the user-defined names will be alongside names in the generated code that follow that convention.

================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:19
+
+message LookupReply { string Symbol_YAML = 1; }
----------------
`symbol_yaml`, lowercase

(this is how all the generated code looks and conventional for protobufs)

================
Comment at: clang-tools-extra/clangd/index/remote/README.md:3
+
+Clangd global index used for code completion, code navigation and a number of
+useful features takes a long time to build (3-4 hours on powerful machines for
----------------
This first sentence doesn't parse (missing a verb and some commas) but should really be split. It should probably avoid so many parentheses too. Maybe:

Clangd uses a global index for project-wide code completion, navigation and other features.
For large projects, building this can take many hours and keeping it loaded uses a lot of memory.

(I don't think the last sentence is needed, it's implied)

================
Comment at: clang-tools-extra/clangd/index/remote/README.md:5
+useful features takes a long time to build (3-4 hours on powerful machines for
+Chrome-sized projects) and induces a significant memory overhead (up to 2 GB)
+to serve. As a result, developers in large open source projects like LLVM often
----------------
("overhead" implies it's not used directly for the task at hand, so it's not really accurate here)

================
Comment at: clang-tools-extra/clangd/index/remote/README.md:7
+to serve. As a result, developers in large open source projects like LLVM often
+don't get the best experience for editing and navigating source code.
+
----------------
Vague references to "the best experience" sound like bad marketing, we should be more specific (or just use fewer words)

================
Comment at: clang-tools-extra/clangd/index/remote/README.md:10
+To relieve that burden, we're building remote index — a global index
+served on a different machine. This directory contains code that is used as
+Proof of Concept for the upcoming remote index feature.
----------------
and shared between developers

================
Comment at: clang-tools-extra/clangd/index/remote/README.md:18
+
+Regardless of the dependencies installation way, to enable this feature and
+build remote index tools you will need to set this CMake flag —
----------------
However you install the dependencies, ...

(or at least "way" -> "method")

================
Comment at: clang-tools-extra/clangd/index/remote/README.md:54
+After installation, you need to 
+
+By default, CMake will look for system-installed libraries when building remote
----------------
drop dead sentence?

================
Comment at: clang-tools-extra/clangd/index/remote/README.md:62
+
+Right now it is only possible to connect to the index service hosted on the
+same machine — `clangd-remote-server`. `clangd-remote-index` is a simple
----------------
I wouldn't lead with the same-machine limitation, the headline is this doesn't work at all with clangd yet.

I also don't think it's worth going into detail about the proof-of-concept: they aren't really interesting to anyone else and will be replaced soon.

Maybe just: the remote index isn't usable with clangd yet, but you can try the proof-of-concept tools in client/ and server/ subdirectories.

================
Comment at: clang-tools-extra/clangd/index/remote/client/CMakeLists.txt:7
+  PRIVATE
+  clangBasic
+  )
----------------
why does this link to clangdbasic?

I forget how cmake transitive deps work, but surely it either needs clangDaemon only, or clangDaemon and all of its deps?

================
Comment at: llvm/CMakeLists.txt:378

+# FIXME(kirillbobyrev): Document this in the LLVM docs once remote index is stable.
+option(CLANGD_ENABLE_REMOTE "Use gRPC library to enable remote index support for Clangd" OFF)
----------------
this code all belongs in clangd cmakelists

================
Comment at: llvm/cmake/modules/FindGRPC.cmake:26
+# Libraries that use these headers should adjust the include path.
+# FIXME(kirillbobyrev): Allow optional generation of prptos only and give
+# callers control over it via additional parameters.
----------------
optional generation of protos only seems backwards - optional generation of grpc code?

================
Comment at: llvm/cmake/modules/FindGRPC.cmake:28
+# callers control over it via additional parameters.
+function(generate_grpc_protos ProtoFile LibraryName)
+  get_filename_component(ProtoSourceAbsolutePath "${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)
----------------
lib name is probably the conventional first arg?
also works better if we're going to allow multiple input files in future

================
Comment at: llvm/cmake/modules/FindGRPC.cmake:46
+
+  add_library(${LibraryName} ${GeneratedProtoSource} ${GeneratedProtoHeader} ${GeneratedGRPCSource} ${GeneratedGRPCHeader})
+  target_link_libraries(${LibraryName} grpc++ protobuf)
----------------
what's the effect of adding the headers to the library?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77794