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