sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:14
+#include "Index.grpc.pb.h"
#include "SourceCode.h"
----------------
this include and the stuff depending on it needs to be ifdef'd
================
Comment at: clang-tools-extra/clangd/index/remote/CMakeLists.txt:1
-generate_grpc_protos(RemoteIndexProtos "Index.proto")
+generate_grpc_protos(RemoteIndexProtos "Index.proto" RemoteProtosLocation)
----------------
why is this extra param needed?
================
Comment at: clang-tools-extra/clangd/index/remote/CMakeLists.txt:9
+add_definitions(-DGOOGLE_PROTOBUF_NO_RTTI=1)
+add_clang_library(clangDaemonRemoteIndex
+ Index.cpp
----------------
let's avoid propagating the clangDaemon name any further. clangdRemoteIndex? or
clangdRemoteIndexClient?
================
Comment at: clang-tools-extra/clangd/index/remote/Index.h:21
+
+class IndexClient : public SymbolIndex {
+public:
----------------
the class doesn't need to be exposed here, as the SymbolIndex interface seems
to do everything we need. Just expose a factory?
Actually, I think that means we can expose this header whether grpc is enabled
or not. If no grpc, then we just link in an implementation of the factory that
always returns an error. WDYT?
================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:16
+
+ rpc FuzzyFind(FuzzyFindRequest) returns (FuzzyFindReply) {}
+}
----------------
should also return a stream. has_more is annoying, but you can keep it in the
message and set it only on the last element of the stream.
================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:37
+message FuzzyFindReply {
+ // FIXME(kirillbobyrev): Convert to Symbol.
+ repeated string symbols = 1;
----------------
confused... why not use Symbol now?
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:44
+clangd::FuzzyFindRequest deserialize(const FuzzyFindRequest *Request) {
+ clangd::FuzzyFindRequest Result;
----------------
move all the conversions into a file, marshalling.cpp or whatever?
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
