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