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

Wed Apr 22 08:06:17 PDT 2020

sammccall added inline comments.

================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:161
 if (CLANGD_ENABLE_REMOTE)
+  add_definitions(-D CLANGD_REMOTE)
   include(FindGRPC)
----------------
Can we use features.inc.in instead for this?
(I don't have a strong opinion on which one is better, but I'd rather not do things two ways)

================
Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:40
+    RemoteMode("remote",
+               llvm::cl::desc("Connect to <INDEX LOCATION> remote index"));
+
----------------
kbobyrev wrote:
> @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.
I think you need to read it before we `ResetCommandLineParser()`. That part is a bit of a hack :-(

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

----------------
kbobyrev wrote:
> sammccall wrote:
> > why is this extra param needed?
> Because this needs to be included both here and for `dexp` binary.
still? I thought the idea was stuff outside this directory could include the client header, whether it's compiled in or not, and that header doesn't require protos

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

================
Comment at: clang-tools-extra/clangd/index/remote/CMakeLists.txt:15
+
+  protobuf
+  grpc++
----------------
do protobuf/grpc++ not get linked in transitively by linking against RemoteIndexProtos? Sad :-(

================
Comment at: clang-tools-extra/clangd/index/remote/Index.cpp:36
+    while (Reader->Read(&Reply))
+      Callback(symbolFromYAML(Reply.yaml_serializatiton(), &Strings));
+    grpc::Status Status = Reader->Finish();
----------------
error-handling: what if the symbol is invalid? (log and skip)

================
Comment at: clang-tools-extra/clangd/index/remote/Index.cpp:36
+    while (Reader->Read(&Reply))
+      Callback(symbolFromYAML(Reply.yaml_serializatiton(), &Strings));
+    grpc::Status Status = Reader->Finish();
----------------
sammccall wrote:
> error-handling: what if the symbol is invalid? (log and skip)
this should call a function in marshalling, which just does the YAML thing for now

================
Comment at: clang-tools-extra/clangd/index/remote/Index.cpp:38
+    grpc::Status Status = Reader->Finish();
+    llvm::outs() << "lookup rpc " << (Status.ok() ? "succeeded" : "failed")
+                 << '\n';
----------------
use log()/vlog if you want to log.
But this should probably be a trace::Span instead so you get the latency.

================
Comment at: clang-tools-extra/clangd/index/remote/Index.cpp:47
+
+    grpc::ClientContext Context;
+    std::unique_ptr<grpc::ClientReader<remote::FuzzyFindReply>> Reader(
----------------
I'd consider pulling out a streamRPC template, these tend to attract cross-cutting code (setting deadlines, tracing/logging, and such)

================
Comment at: clang-tools-extra/clangd/index/remote/Index.cpp:97
+std::unique_ptr<SymbolIndex> connect(llvm::StringRef Address) {
+#ifdef CLANGD_REMOTE
+  return std::unique_ptr<SymbolIndex>(new IndexClient(Address));
----------------
if  remote is disabled we can't compile the rest of this file.
Rather than ifdefs here, I'd suggest doing it at the cmake level:

```
if(CLANGD_ENABLE_REMOTE)
  add_clang_library(... Index.cpp)
else()
  add_clang_library(... IndexUnimplemented.cpp)
endif()
```

================
Comment at: clang-tools-extra/clangd/index/remote/Index.h:1
+//===--- Index.h -------------------------------------------------*- C++-*-===//
+//
----------------
Suggest calling this Client.h

================
Comment at: clang-tools-extra/clangd/index/remote/Index.h:12
+
+#include "grpcpp/grpcpp.h"
+
----------------
no implementation headers should be needed now

================
Comment at: clang-tools-extra/clangd/index/remote/Index.h:21
+
+std::unique_ptr<SymbolIndex> connect(llvm::StringRef Address);
+
----------------
this needs docs:
 - what it does
 - error conditions
 - what happens if support isn't compiled in

================
Comment at: clang-tools-extra/clangd/index/remote/Index.h:21
+
+std::unique_ptr<SymbolIndex> connect(llvm::StringRef Address);
+
----------------
sammccall wrote:
> this needs docs:
>  - what it does
>  - error conditions
>  - what happens if support isn't compiled in
is the address resolved synchronously? is there a timeout?
does this block until the channel is ready, or will that happen on the first request?

(My sense is you're probably going to want a timeout param and to describe what it does, and *maybe* return different error types)

================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:13

 service Index {
+  rpc Lookup(LookupRequest) returns (stream Symbol) {}
----------------
Just realized: we should probably call this SymbolIndex to match clangd::SymbolIndex (or rename the latter to match this)

================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:21
+
+message LookupRequest { repeated string ids = 1; }
+
----------------
nit: I'd suggest grouping requests with corresponding replies, and all the common vocab types either above or below.

================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:36

-message LookupRequest { string id = 1; }
+// Actual messages contain only symbols and they will be followed by a
+// terminating message with has_more field.
----------------
"Actual messages" is a bit vague.
Maybe "The response is a stream of `symbol` messages, and one terminating `has_more` message."

================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:39
+message FuzzyFindReply {
+  oneof symbol_or_has_more {
+    Symbol symbol = 1;
----------------
I think a generic name like `kind` would be more readable here.

If the stream/final-result pattern is going to be our common one, you could consider giving them *all* generic names, like:
```
message FuzzyFindReply {
  oneof kind {
    Symbol stream_result;
    bool final_result; // HasMore
  }
}
```

Then they conform to a `Stream<StreamT, FinalT>` concept and you can abstract the handling in a template.

This makes sense if we want to lean into the index service being a mechanical translation of clangd::SymbolIndex. But I'm not sure whether that's a good idea, e.g. this one needs to be backwards-compatible, so they may drift.

================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:55
+  oneof ref_or_has_more {
+    string ref = 1;
+    bool has_more = 2;
----------------
Ref should be a message with ref_yaml for now?
(Consistency with Symbol)

================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:37
+message FuzzyFindReply {
+  // FIXME(kirillbobyrev): Convert to Symbol.
+  repeated string symbols = 1;
----------------
kbobyrev wrote:
> 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.
Did you find out? Fixing this isn't a backwards-compatible change, and "this code is broken and I don't know why" isn't a great state for checkin.

Happy to help but please provide some details.

================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:76
+      FuzzyFindReply NextMessage;
+      Symbol *NextSymbol = new Symbol;
+      NextSymbol->set_yaml_serializatiton(toYAML(Sym));
----------------
new and set_allocated_* are scary :-)

NextMessage.mutable_symbol()->set_yaml_serialization(...)

================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:89
+                    grpc::ServerWriter<RefsReply> *Reply) override {
+    clangd::RefsRequest Req;
+    for (const auto &ID : Request->ids()) {
----------------
fromProtobuf(Request)?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78521