sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:13
+import "google/protobuf/empty.proto";
import "Index.proto";
----------------
question of style, but unless there's a concrete benefit I'd suggest just
defining the request inline.
Cost is not just the import but also the irregularity of the request type *not*
being called MonitoringInfoRequest or whatever.
================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:16
+message MonitoringInfoReply { optional string info = 1; }
+
----------------
it seems unlikely to me that `string info` is the right format, but I can't
tell whether this is a placeholder or not
================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:29
+
+ rpc MonitoringInfo(google.protobuf.Empty) returns (MonitoringInfoReply) {}
}
----------------
Who's going to call this? If clients will, then it probably belongs here, but
should have a different name.
If only e.g. a web UI or monitoring system will, it belongs in a separate
`service` IMO.
Depending on what it returns, it may be interesting for clients to call and log!
Or it may be useful to ban clients from calling it (if it has private details)
================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:29
+
+ rpc MonitoringInfo(google.protobuf.Empty) returns (MonitoringInfoReply) {}
}
----------------
sammccall wrote:
> Who's going to call this? If clients will, then it probably belongs here, but
> should have a different name.
> If only e.g. a web UI or monitoring system will, it belongs in a separate
> `service` IMO.
>
> Depending on what it returns, it may be interesting for clients to call and
> log!
> Or it may be useful to ban clients from calling it (if it has private details)
I think we should try harder to find a name that describes the data requested,
rather than what we expect the data to be used for.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:332
grpc::EnableDefaultHealthCheckService(true);
+ grpc::reflection::InitProtoReflectionServerBuilderPlugin();
grpc::ServerBuilder Builder;
----------------
This seems unrelated. Because it requires a build dep, move it to a separate
patch? (That probably doesn't need review)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98246/new/
https://reviews.llvm.org/D98246
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
