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