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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev marked 5 inline comments as done.
kbobyrev added inline comments.


================
Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:14
 
+#include "Index.grpc.pb.h"
 #include "SourceCode.h"
----------------
sammccall wrote:
> this include and the stuff depending on it needs to be ifdef'd
Ah, right, I forgot about this. Was present in the previous patch where I had custom definitions, but forgot about it for some reason.


================
Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:40
+    RemoteMode("remote",
+               llvm::cl::desc("Connect to <INDEX LOCATION> remote index"));
+
----------------
@sammccall do you know why this opt is not being set for some reason? When I try to run `dexp --help` it is displayed in the possible arguments and when I try to pass anything invalid for boolean flags this also fails, but the value is not changed regardless of the values I put here :( Must be something simple I've missed.


================
Comment at: clang-tools-extra/clangd/index/remote/CMakeLists.txt:1
-generate_grpc_protos(RemoteIndexProtos "Index.proto")
+generate_grpc_protos(RemoteIndexProtos "Index.proto" RemoteProtosLocation)
 
----------------
sammccall wrote:
> why is this extra param needed?
Because this needs to be included both here and for `dexp` binary.


================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:16
+
+  rpc FuzzyFind(FuzzyFindRequest) returns (FuzzyFindReply) {}
+}
----------------
sammccall wrote:
> 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.
Is this an idiomatic way of using gRPC + Protobuf? Seems counter-intuitive to me since repeated stream of symbols in the message + single `has_more` seems quite logical.


================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:37
+message FuzzyFindReply {
+  // FIXME(kirillbobyrev): Convert to Symbol.
+  repeated string symbols = 1;
----------------
sammccall wrote:
> confused... why not use Symbol now?
Couldn't put `Symbol`s into `FuzzyFindReply` for some reason. Clangd behaves really weird with Protobuf inclusions for me... Need to figure out why that happens, but might be me doing something wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78521