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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG, nits apart from the llvm_unreachable in unimplemented, let's discuss further if you disagree on that.

We do have to go back and add unit tests for this stuff. I'm OK with doing that once the marshalling is sorted out (not YAML), but we should make sure we remember!



================
Comment at: clang-tools-extra/clangd/index/YAMLSerialization.cpp:437
+  if (YAMLInput.error())
+    return llvm::None;
+  YAMLInput >> Deserialized;
----------------
we should still log (or at least vlog)

(Actually personally I'd return Expected here and then log and drop to optional in Marshalling, but it doesn't matter)


================
Comment at: clang-tools-extra/clangd/index/dex/dexp/CMakeLists.txt:2
 include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../../../)
+# Include Features.inc
+include_directories(${CMAKE_CURRENT_BINARY_DIR}/../../../)
----------------
This comment is likely to get stale. I'd drop it: if we're including the clangd source dir, also including the clangd binary dir is appropriate and doesn't really require an explanation.


================
Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:32
+llvm::cl::opt<std::string>
+    IndexLocation(llvm::cl::desc("<path to Dex | remote:server.address>"),
+                  llvm::cl::Positional);
----------------
Dex -> index file
(dex is the in-memory implementation, not the data format)


================
Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:44
 
+You can connect to remote index by passing remote:address to dex. Example:
+
----------------
dex -> dexp (and in the example command)


================
Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:280
 std::unique_ptr<SymbolIndex> openIndex(llvm::StringRef Index) {
+#ifdef CLANGD_ENABLE_REMOTE
+  return Index.startswith("remote:")
----------------
no longer needs to be ifdef'd: if the user passes remote:foo and it's not compiled in, they'll get an appropriate error


================
Comment at: clang-tools-extra/clangd/index/remote/CMakeLists.txt:26
+else()
+  add_subdirectory(unimplemented)
+endif()
----------------
Maybe "# provides a dummy implementation of clangdRemoteIndex"


================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:47
+    if (!Sym)
+      elog("YAML parsing error: {0}", Sym.takeError());
+    Callback(*Sym);
----------------
nit: I'd suggest dropping "YAML" from this message so we don't forget later. Rather `"Received invalid {0}: {1}", ReplyT::descriptor()->name(), Sym.takeError()`


================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:95
+std::unique_ptr<clangd::SymbolIndex> getClient(llvm::StringRef Address) {
+  return std::unique_ptr<clangd::SymbolIndex>(new IndexClient(Address));
+}
----------------
nit: for slightly cleaner separation, I'd suggest creating the channel here and passing it into the IndexClient constructor.

The channel is where various configuration (auth, etc) might live, and it's nice if the client class stays doesn't need to deal with that level of abstraction.
e.g. a test version that doesn't use the real network would pass in a different channel and the IndexClient wouldn't care. (Not sure if that's how testing is actually done in grpc these days, but you get the idea...)


================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:29
+  bool streamRPC(ClangdRequestT Request,
+                 std::function<std::unique_ptr<grpc::ClientReader<ReplyT>>(
+                     remote::SymbolIndex::Stub *, grpc::ClientContext *,
----------------
whether this is a member pointer or a function wrapper, it should be in a typedef or readability.
e.g.
template <typename ReqT, typename RspT>
using StreamingCall = std::function<unique_ptr<ClientReader<RspT>>>(Stub*, ClientContext*, const RspT&);


================
Comment at: clang-tools-extra/clangd/index/remote/Client.h:1
+//===--- Index.h -------------------------------------------------*- C++-*-===//
+//
----------------
Client.h - Connect to a remote index via gRPC


================
Comment at: clang-tools-extra/clangd/index/remote/Index.cpp:27
+    llvm::StringRef SpanName, SymbolIndex::Stub *Stub, ClangdRequestT Request,
+    std::function<std::unique_ptr<grpc::ClientReader<ReplyT>>(
+        SymbolIndex::Stub *, grpc::ClientContext *, const RPCRequestT &)>
----------------
kbobyrev wrote:
> sammccall wrote:
> > Hmm, I think member pointers are totally the right thing here. And all the params should be deducible.
> > 
> > Have a look at https://godbolt.org/z/AnprJ- (simplified but self-contained and I think shows all the bits)
> > 
> > The thing I can't work out how to do is make the member pointer a template param *and* have it be deduced...
> Uh, I had the pointers but decided against it because of rather... bizarre syntax :D
> 
> And yes, just like you said, the calls contained too many explicit template parameters, so I decided the implementation should be more verbose than "user code". I'm not against pointers, but I think the current version looks simpler, WDYT?
There are advantages of member function pointers here:
 - the fact that it's a member function on the stub *is* significant information here I think
 - passing a std::function allows deduction of `RPCRequestT` and `ReplyT` as the compiler can pattern-match, while that doesn't work for `function<>` because there's an implicit conversion in the way.
 - (std::function is slow, but that shouldn't matter here. I guess function_ref doesn't have support for member functions?)

> And yes, just like you said, the calls contained too many explicit template parameters

I think this is backwards, the function pointer version should require no explicit type template parameters, where the function<> version requires 2. See the godbolt example above for deduction.

(The thing I couldn't get working was making the member fptr a *non-type-template-param* while keeping the type template params deduced, but that's not a disadvantage vs function<>)

> I think the current version looks simpler, WDYT?
After extracting an alias template for the type (which should happen anyway) I don't think there's a significant difference really:

```
template <typename ReqT, RspT>
using StreamingCall = std::function<std::unique_ptr<grpc::ClientReader<ReplyT>>(Stub*, ClientContext*, const ReqT&)>;
// vs
template <typename ReqT, RspT>
using StreamingCall = std::unique_ptr<grpc::ClientReader<ReplyT>>(Stub::*)(ClientContext*, const ReqT&);
```

the member fptr syntax is more unusual/exotic, but shorter and less nested. Neither is really readable, and the readability of this line isn't that significant.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:64
+      LookupReply NextMessage;
+      NextMessage.mutable_stream_result()->set_yaml_serialization(toYAML(Sym));
+      Reply->Write(NextMessage);
----------------
*NextMessage.mutable_stream_result() = toProtobuf(Sym)

and move the trivial `toProtobuf` logic to Marshalling (so it's trivial to replace later)


================
Comment at: clang-tools-extra/clangd/index/remote/unimplemented/CMakeLists.txt:1
+include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../../../)
+add_clang_library(clangdRemoteIndex
----------------
This is unusual, include a comment for what it's doing.


================
Comment at: clang-tools-extra/clangd/index/remote/unimplemented/Client.cpp:1
+//===--- Client.cpp ----------------------------------------------*- C++-*-===//
+//
----------------
Probably call this UnimplementedClient.cpp, no need to have the source filenames unneccesarily coincide? Makes it clear the other one is the primary.


================
Comment at: clang-tools-extra/clangd/index/remote/unimplemented/Client.cpp:19
+      "Can't create SymbolIndex client without Remote Index support.";
+  llvm_unreachable(Message);
+  elog(Message);
----------------
no llvm_unreachable, we shouldn't crash. Just elog and continue...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78521