sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/Features.inc.in:2
#define CLANGD_BUILD_XPC @CLANGD_BUILD_XPC@
+#define CLANGD_REMOTE @CLANGD_ENABLE_REMOTE@
----------------
nit: can we use the same name for the cmake variable and the preprocessor
define?
There's a lot of potential for conceptual confusion between them, but I don't
think giving them subtly different names helps, I'd rather make it just work
for confused people :-)
================
Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:14
+#include "Features.inc.in"
#include "SourceCode.h"
----------------
Features.inc - that's the version that's been cmake-substituted
(I'm not sure why this isn't a regular include-guarded header, let's fix that
separately)
================
Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:18
#include "index/dex/Dex.h"
+#include "index/remote/Client.h"
#include "llvm/ADT/ScopeExit.h"
----------------
Being able to include this header but not call the function in it doesn't make
sense - linker errors aren't a friendly way to surface this constraint.
If the goal is to forbid use of the API without it compiled in, the header
should `#ifndef #CLANGD_ENABLE_REMOTE #error "Client.h included but
CLANGD_ENABLE_REMOTE is off" #endif`, and all the include-sites should be
#ifdef-guarded.
(But please see other comments first)
================
Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:43
+// FIXME(kirillbobyrev): This is not working properly for some reason.
+llvm::cl::opt<bool>
+ RemoteMode("remote",
----------------
I'd consider rather making this a string prefix on `IndexLocation`, so you'd
invoke as `dexp remote:localhost:4004` or whatever.
This encapsulates more easily, e.g. we can eventually move this into
`openIndex` so all the callers transparently support the remote index.
(Though we shouldn't push that dependency into clangd in *this* patch, it just
makes it easier in future)
================
Comment at: clang-tools-extra/clangd/index/remote/Client.h:21
+///
+/// Caller is not blocked upon SymbolIndex request, the actual connection must
+/// happen upon the first request. RPCs have no timeout.
----------------
I can't follow this sentence - it's not clear what "SymbolIndex request" or
"the actual connection" are.
I guess you mean something like "This method blocks to resolve the address, but
does not wait for the channel to become ready".
================
Comment at: clang-tools-extra/clangd/index/remote/Client.h:21
+///
+/// Caller is not blocked upon SymbolIndex request, the actual connection must
+/// happen upon the first request. RPCs have no timeout.
----------------
sammccall wrote:
> I can't follow this sentence - it's not clear what "SymbolIndex request" or
> "the actual connection" are.
>
> I guess you mean something like "This method blocks to resolve the address,
> but does not wait for the channel to become ready".
>
The blocking behaviour is interesting:
- clangd should keep working while offline or in bad connectivity (thus no
timeout on RPCs is a bug, we shouldn't rely on the channel outright detecting
failure)
- this implies you don't want to return null if the connection can't be
established, and so there's indeed not much point blocking. But blocking on
resolving the name removes a lot of the benefit here.
I think GRPC actually has a pretty good model, it's documented here:
https://github.com/grpc/grpc/blob/master/doc/connectivity-semantics-and-api.md
(may be worth adding a link).
I'm not sure if this is actually blocked on name resolution, but it seems like
it's at most blocked on *one attempt* at name resolution: the channel can still
be created if resolution hasn't succeeded yet.
================
Comment at: clang-tools-extra/clangd/index/remote/Client.h:22
+/// Caller is not blocked upon SymbolIndex request, the actual connection must
+/// happen upon the first request. RPCs have no timeout.
+///
----------------
RPCs have no timeout should be a FIXME
================
Comment at: clang-tools-extra/clangd/index/remote/Client.h:29
+/// function's implementation will not be compiled.
+/// * Even though the address is resolved during the function call, the server
+/// availability is only required during RPC calls.
----------------
I'm not sure what this is saying or why a caller would want to know it.
================
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));
----------------
kbobyrev wrote:
> sammccall wrote:
> > 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()
> > ```
> I would argue that including `index/remote/Client.h` without
> `CLANGD_ENABLE_REMOTE` is not correct. We would still have to put `#ifdefs`
> in the user code regardless of whether what you proposed is implemented. I
> don't see any benefits in allowing users to include `index/remote/Client.h`,
> link `clangdRemoteIndex` and getting a runtime error. All of those steps have
> _remote_ in them and if _remote mode_ is not enabled, something certainly
> went wrong.
>
> Also, this will complicate CMake structure as I can't put files that are
> conditionally added/excluded from the clang library and I would either have
> to make a separate directory with an "empty" library, or put a bunch of
> `#ifdefs` in here. Either is not optimal and I think it'd be better to leave
> it like this. WDYT?
> I would argue that including index/remote/Client.h without
> CLANGD_ENABLE_REMOTE is not correct. We would still have to put #ifdefs in
> the user code regardless of whether what you proposed is implemented
This is begging the question - what I'm proposing is to give that
header/library defined behavior when CLANGD_ENABLE_REMOTE is off, and so you
wouldn't need #ifdefs.
> I don't see any benefits in allowing users to include index/remote/Client.h,
> link clangdRemoteIndex and getting a runtime error.
The benefits are:
- no ifdefs and fewer conditional cmake rules, which are both hard to read and
result in untested constructs without even basic syntax checking
- fewer cases to consider in clients, as this falls into the regular "can't
connect to index" codepath
- readers trying to follow the possible behaviours end up at the header
documentation of a function that is called, which is an easy and familiar
workflow
> All of those steps have _remote_ in them and if _remote mode_ is not enabled,
> something certainly went wrong
Again, this is begging the question. If the header says "if GRPC mode is not
enabled, always returns nullptr", then everything is working as designed.
> Also, this will complicate CMake structure as I can't put files that are
> conditionally added/excluded from the clang library
Why not? (This sounds like a fair technical objection, but may be surmountable)
================
Comment at: clang-tools-extra/clangd/index/remote/Index.cpp:1
+//===--- Index.cpp -----------------------------------------------*-
C++-*-===//
+//
----------------
This implements Client.h, should be Client.cpp
================
Comment at: clang-tools-extra/clangd/index/remote/Index.cpp:14
+#include "Trace.h"
+#include "grpcpp/grpcpp.h"
+#include "index/Serialization.h"
----------------
I'd consider using `<> for this include, or at least splitting it into a
different section - non-LLVM non-stdlib includes are really rare.
================
Comment at: clang-tools-extra/clangd/index/remote/Index.cpp:26
+void streamRPC(
+ llvm::StringRef SpanName, SymbolIndex::Stub *Stub, ClangdRequestT Request,
+ std::function<std::unique_ptr<grpc::ClientReader<ReplyT>>(
----------------
no need to pass the stub if you make this a member func
================
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 &)>
----------------
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...
================
Comment at: clang-tools-extra/clangd/index/remote/Index.cpp:31
+ llvm::function_ref<void(ReplyT, llvm::UniqueStringSaver *)> Callback) {
+ trace::Span Tracer(SpanName);
+ const auto RPCRequest = toProtobuf(Request);
----------------
SpanName can be RPCRequestT::descriptor()->name() (e.g. "LookupRequest") rather
than having to pass it explicitly
================
Comment at: clang-tools-extra/clangd/index/remote/Index.cpp:46
+ IndexClient(llvm::StringRef Address)
+ : Stub(remote::SymbolIndex::NewStub(grpc::CreateChannel(
+ Address.str(), grpc::InsecureChannelCredentials()))) {}
----------------
not even attempting to open the connection on startup seems to introduce more
latency on first request than necessary. Call Channel->GetState(true)?
That won't block, but it will cause the connection to establish in the
background.
================
Comment at: clang-tools-extra/clangd/index/remote/Index.cpp:68
+ &remote::SymbolIndex::Stub::FuzzyFind,
+ [&](const FuzzyFindReply &Message, llvm::UniqueStringSaver *Strings) {
+ if (!Message.has_stream_result()) {
----------------
this callback is almost the same - if you added a dummy stream result to Lookup
you could move it into streamrpc :-)
================
Comment at: clang-tools-extra/clangd/index/remote/Marshalling.h:25
+clangd::FuzzyFindRequest fromProtobuf(const FuzzyFindRequest *Request);
+llvm::Expected<clangd::Symbol> fromProtobuf(const Symbol &Message,
+ llvm::UniqueStringSaver *Strings);
----------------
You could also just log the error and return Optional at this level... there's
no recovery other than logging anyway. I kind of hate Expected/Error because of
their asserting and weird non-constness so I'd be tempted by that.
Up to you though
================
Comment at: clang-tools-extra/clangd/index/remote/server/CMakeLists.txt:17
- protobuf
- grpc++
- clangDaemon
+ clangdRemoteIndex
)
----------------
why does the server depend on the client?
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
