[PATCH] D78521: [clangd] Extend dexp to support remote index

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:14
 
+#include "Index.grpc.pb.h"
 #include "SourceCode.h"
----------------
this include and the stuff depending on it needs to be ifdef'd


================
Comment at: clang-tools-extra/clangd/index/remote/CMakeLists.txt:1
-generate_grpc_protos(RemoteIndexProtos "Index.proto")
+generate_grpc_protos(RemoteIndexProtos "Index.proto" RemoteProtosLocation)
 
----------------
why is this extra param needed?


================
Comment at: clang-tools-extra/clangd/index/remote/CMakeLists.txt:9
+add_definitions(-DGOOGLE_PROTOBUF_NO_RTTI=1)
+add_clang_library(clangDaemonRemoteIndex
+  Index.cpp
----------------
let's avoid propagating the clangDaemon name any further. clangdRemoteIndex? or clangdRemoteIndexClient?


================
Comment at: clang-tools-extra/clangd/index/remote/Index.h:21
+
+class IndexClient : public SymbolIndex {
+public:
----------------
the class doesn't need to be exposed here, as the SymbolIndex interface seems to do everything we need. Just expose a factory?

Actually, I think that means we can expose this header whether grpc is enabled or not. If no grpc, then we just link in an implementation of the factory that always returns an error. WDYT?


================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:16
+
+  rpc FuzzyFind(FuzzyFindRequest) returns (FuzzyFindReply) {}
+}
----------------
should also return a stream. has_more is annoying, but you can keep it in the message and set it only on the last element of the stream.


================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:37
+message FuzzyFindReply {
+  // FIXME(kirillbobyrev): Convert to Symbol.
+  repeated string symbols = 1;
----------------
confused... why not use Symbol now?


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:44
 
+clangd::FuzzyFindRequest deserialize(const FuzzyFindRequest *Request) {
+  clangd::FuzzyFindRequest Result;
----------------
move all the conversions into a file, marshalling.cpp or whatever?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78521