[lldb] Adding A new Binding helper for JSONTransport. #159160
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
ashgti
force-pushed
the
lldb-jsontransport-binder
branch
5 times, most recently
from
578fe6f to
b91cad6
Compare
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesThis adds a new Binding helper class to allow mapping of incoming and outgoing requests / events to specific handlers. This should make it easier to create new protocol implementations and allow us to create a relay in the lldb-mcp binary. Patch is 95.85 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/159160.diff 18 Files Affected:
diff --git a/lldb/include/lldb/Host/JSONTransport.h b/lldb/include/lldb/Host/JSONTransport.h
index 210f33edace6e..da1ae43118538 100644
--- a/lldb/include/lldb/Host/JSONTransport.h
+++ b/lldb/include/lldb/Host/JSONTransport.h
@@ -18,6 +18,7 @@
#include "lldb/Utility/IOObject.h"
#include "lldb/Utility/Status.h"
#include "lldb/lldb-forward.h"
+#include "llvm/ADT/FunctionExtras.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
@@ -25,8 +26,13 @@
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/JSON.h"
#include "llvm/Support/raw_ostream.h"
+#include <functional>
+#include <mutex>
+#include <optional>
#include <string>
#include <system_error>
+#include <type_traits>
+#include <utility>
#include <variant>
#include <vector>
@@ -50,17 +56,70 @@ class TransportUnhandledContentsError
std::string m_unhandled_contents;
};
+class InvalidParams : public llvm::ErrorInfo<InvalidParams> {
+public:
+ static char ID;
+
+ explicit InvalidParams(std::string method, std::string context)
+ : m_method(std::move(method)), m_context(std::move(context)) {}
+
+ void log(llvm::raw_ostream &OS) const override;
+ std::error_code convertToErrorCode() const override;
+
+private:
+ std::string m_method;
+ std::string m_context;
+};
+
+// Value for tracking functions that have a void param or result.
+using VoidT = std::monostate;
+
+template <typename T> using Callback = llvm::unique_function<T>;
+
+template <typename T>
+using Reply = typename std::conditional<
+ std::is_same_v<T, VoidT> == true, llvm::unique_function<void(llvm::Error)>,
+ llvm::unique_function<void(llvm::Expected<T>)>>::type;
+
+template <typename Result, typename Params>
+using OutgoingRequest = typename std::conditional<
+ std::is_same_v<Params, VoidT> == true,
+ llvm::unique_function<void(Reply<Result>)>,
+ llvm::unique_function<void(const Params &, Reply<Result>)>>::type;
+
+template <typename Params>
+using OutgoingEvent = typename std::conditional<
+ std::is_same_v<Params, VoidT> == true, llvm::unique_function<void()>,
+ llvm::unique_function<void(const Params &)>>::type;
+
+template <typename Id, typename Req>
+Req make_request(Id id, llvm::StringRef method,
+ std::optional<llvm::json::Value> params = std::nullopt);
+template <typename Req, typename Resp>
+Resp make_response(const Req &req, llvm::Error error);
+template <typename Req, typename Resp>
+Resp make_response(const Req &req, llvm::json::Value result);
+template <typename Evt>
+Evt make_event(llvm::StringRef method,
+ std::optional<llvm::json::Value> params = std::nullopt);
+template <typename Resp>
+llvm::Expected<llvm::json::Value> get_result(const Resp &resp);
+template <typename Id, typename T> Id get_id(const T &);
+template <typename T> llvm::StringRef get_method(const T &);
+template <typename T> llvm::json::Value get_params(const T &);
+
/// A transport is responsible for maintaining the connection to a client
/// application, and reading/writing structured messages to it.
///
/// Transports have limited thread safety requirements:
/// - Messages will not be sent concurrently.
/// - Messages MAY be sent while Run() is reading, or its callback is active.
-template <typename Req, typename Resp, typename Evt> class Transport {
+template <typename Id, typename Req, typename Resp, typename Evt>
+class JSONTransport {
public:
using Message = std::variant<Req, Resp, Evt>;
- virtual ~Transport() = default;
+ virtual ~JSONTransport() = default;
/// Sends an event, a message that does not require a response.
virtual llvm::Error Send(const Evt &) = 0;
@@ -90,8 +149,6 @@ template <typename Req, typename Resp, typename Evt> class Transport {
virtual void OnClosed() = 0;
};
- using MessageHandlerSP = std::shared_ptr<MessageHandler>;
-
/// RegisterMessageHandler registers the Transport with the given MainLoop and
/// handles any incoming messages using the given MessageHandler.
///
@@ -100,22 +157,302 @@ template <typename Req, typename Resp, typename Evt> class Transport {
virtual llvm::Expected<MainLoop::ReadHandleUP>
RegisterMessageHandler(MainLoop &loop, MessageHandler &handler) = 0;
- // FIXME: Refactor mcp::Server to not directly access log on the transport.
- // protected:
+protected:
template <typename... Ts> inline auto Logv(const char *Fmt, Ts &&...Vals) {
Log(llvm::formatv(Fmt, std::forward<Ts>(Vals)...).str());
}
virtual void Log(llvm::StringRef message) = 0;
+
+ /// Function object to reply to a call.
+ /// Each instance must be called exactly once, otherwise:
+ /// - the bug is logged, and (in debug mode) an assert will fire
+ /// - if there was no reply, an error reply is sent
+ /// - if there were multiple replies, only the first is sent
+ class ReplyOnce {
+ std::atomic<bool> replied = {false};
+ const Req req;
+ JSONTransport *transport; // Null when moved-from.
+ JSONTransport::MessageHandler *handler; // Null when moved-from.
+
+ public:
+ ReplyOnce(const Req req, JSONTransport *transport,
+ JSONTransport::MessageHandler *handler)
+ : req(req), transport(transport), handler(handler) {
+ assert(handler);
+ }
+ ReplyOnce(ReplyOnce &&other)
+ : replied(other.replied.load()), req(other.req),
+ transport(other.transport), handler(other.handler) {
+ other.transport = nullptr;
+ other.handler = nullptr;
+ }
+ ReplyOnce &operator=(ReplyOnce &&) = delete;
+ ReplyOnce(const ReplyOnce &) = delete;
+ ReplyOnce &operator=(const ReplyOnce &) = delete;
+
+ ~ReplyOnce() {
+ if (transport && handler && !replied) {
+ assert(false && "must reply to all calls!");
+ (*this)(make_response<Req, Resp>(
+ req, llvm::createStringError("failed to reply")));
+ }
+ }
+
+ void operator()(const Resp &resp) {
+ assert(transport && handler && "moved-from!");
+ if (replied.exchange(true)) {
+ assert(false && "must reply to each call only once!");
+ return;
+ }
+
+ if (llvm::Error error = transport->Send(resp))
+ handler->OnError(std::move(error));
+ }
+ };
+
+public:
+ class Binder;
+ using BinderUP = std::unique_ptr<Binder>;
+
+ /// Binder collects a table of functions that handle calls.
+ ///
+ /// The wrapper takes care of parsing/serializing responses.
+ class Binder : public JSONTransport::MessageHandler {
+ public:
+ explicit Binder(JSONTransport &transport)
+ : m_transport(transport), m_seq(0) {}
+
+ Binder(const Binder &) = delete;
+ Binder &operator=(const Binder &) = delete;
+
+ /// Bind a handler on transport disconnect.
+ template <typename Fn, typename... Args>
+ void disconnected(Fn &&fn, Args &&...args) {
+ m_disconnect_handler = [&, args...]() mutable {
+ std::invoke(std::forward<Fn>(fn), std::forward<Args>(args)...);
+ };
+ }
+
+ /// Bind a handler on error when communicating with the transport.
+ template <typename Fn, typename... Args>
+ void error(Fn &&fn, Args &&...args) {
+ m_error_handler = [&, args...](llvm::Error error) mutable {
+ std::invoke(std::forward<Fn>(fn), std::forward<Args>(args)...,
+ std::move(error));
+ };
+ }
+
+ template <typename T>
+ static llvm::Expected<T> parse(const llvm::json::Value &raw,
+ llvm::StringRef method) {
+ T result;
+ llvm::json::Path::Root root;
+ if (!fromJSON(raw, result, root)) {
+ // Dump the relevant parts of the broken message.
+ std::string context;
+ llvm::raw_string_ostream OS(context);
+ root.printErrorContext(raw, OS);
+ return llvm::make_error<InvalidParams>(method.str(), context);
+ }
+ return std::move(result);
+ }
+
+ /// Bind a handler for a request.
+ /// e.g. `bind("peek", &ThisModule::peek, this, std::placeholders::_1);`.
+ /// Handler should be e.g. `Expected<PeekResult> peek(const PeekParams&);`
+ /// PeekParams must be JSON parsable and PeekResult must be serializable.
+ template <typename Result, typename Params, typename Fn, typename... Args>
+ void bind(llvm::StringLiteral method, Fn &&fn, Args &&...args) {
+ assert(m_request_handlers.find(method) == m_request_handlers.end() &&
+ "request already bound");
+ if constexpr (std::is_void_v<Params> || std::is_same_v<VoidT, Params>) {
+ m_request_handlers[method] =
+ [fn,
+ args...](const Req &req,
+ llvm::unique_function<void(const Resp &)> reply) mutable {
+ llvm::Expected<Result> result = std::invoke(
+ std::forward<Fn>(fn), std::forward<Args>(args)...);
+ if (!result)
+ return reply(make_response<Req, Resp>(req, result.takeError()));
+ reply(make_response<Req, Resp>(req, toJSON(*result)));
+ };
+ } else {
+ m_request_handlers[method] =
+ [method, fn,
+ args...](const Req &req,
+ llvm::unique_function<void(const Resp &)> reply) mutable {
+ llvm::Expected<Params> params =
+ parse<Params>(get_params<Req>(req), method);
+ if (!params)
+ return reply(make_response<Req, Resp>(req, params.takeError()));
+
+ llvm::Expected<Result> result = std::invoke(
+ std::forward<Fn>(fn), std::forward<Args>(args)..., *params);
+ if (!result)
+ return reply(make_response<Req, Resp>(req, result.takeError()));
+
+ reply(make_response<Req, Resp>(req, toJSON(*result)));
+ };
+ }
+ }
+
+ /// Bind a handler for a event.
+ /// e.g. `bind("peek", &ThisModule::peek, this);`
+ /// Handler should be e.g. `void peek(const PeekParams&);`
+ /// PeekParams must be JSON parsable.
+ template <typename Params, typename Fn, typename... Args>
+ void bind(llvm::StringLiteral method, Fn &&fn, Args &&...args) {
+ assert(m_event_handlers.find(method) == m_event_handlers.end() &&
+ "event already bound");
+ if constexpr (std::is_void_v<Params> || std::is_same_v<VoidT, Params>) {
+ m_event_handlers[method] = [fn, args...](const Evt &) mutable {
+ std::invoke(std::forward<Fn>(fn), std::forward<Args>(args)...);
+ };
+ } else {
+ m_event_handlers[method] = [this, method, fn,
+ args...](const Evt &evt) mutable {
+ llvm::Expected<Params> params =
+ parse<Params>(get_params<Evt>(evt), method);
+ if (!params)
+ return OnError(params.takeError());
+ std::invoke(std::forward<Fn>(fn), std::forward<Args>(args)...,
+ *params);
+ };
+ }
+ }
+
+ /// Bind a function object to be used for outgoing requests.
+ /// e.g. `OutgoingRequest<Params, Result> Edit = bind("edit");`
+ /// Params must be JSON-serializable, Result must be parsable.
+ template <typename Result, typename Params>
+ OutgoingRequest<Result, Params> bind(llvm::StringLiteral method) {
+ if constexpr (std::is_void_v<Params> || std::is_same_v<VoidT, Params>) {
+ return [this, method](Reply<Result> fn) {
+ std::scoped_lock<std::recursive_mutex> guard(m_mutex);
+ Id id = ++m_seq;
+ Req req = make_request<Req, Resp>(id, method, std::nullopt);
+ m_pending_responses[id] = [fn = std::move(fn),
+ method](const Resp &resp) mutable {
+ llvm::Expected<llvm::json::Value> result = get_result<Resp>(resp);
+ if (!result)
+ return fn(result.takeError());
+ fn(parse<Result>(*result, method));
+ };
+ if (llvm::Error error = m_transport.Send(req))
+ OnError(std::move(error));
+ };
+ } else {
+ return [this, method](const Params ¶ms, Reply<Result> fn) {
+ std::scoped_lock<std::recursive_mutex> guard(m_mutex);
+ Id id = ++m_seq;
+ Req req =
+ make_request<Id, Req>(id, method, llvm::json::Value(params));
+ m_pending_responses[id] = [fn = std::move(fn),
+ method](const Resp &resp) mutable {
+ llvm::Expected<llvm::json::Value> result = get_result<Resp>(resp);
+ if (llvm::Error err = result.takeError())
+ return fn(std::move(err));
+ fn(parse<Result>(*result, method));
+ };
+ if (llvm::Error error = m_transport.Send(req))
+ OnError(std::move(error));
+ };
+ }
+ }
+
+ /// Bind a function object to be used for outgoing events.
+ /// e.g. `OutgoingEvent<LogParams> Log = bind("log");`
+ /// LogParams must be JSON-serializable.
+ template <typename Params>
+ OutgoingEvent<Params> bind(llvm::StringLiteral method) {
+ if constexpr (std::is_void_v<Params> || std::is_same_v<VoidT, Params>) {
+ return [this, method]() {
+ if (llvm::Error error =
+ m_transport.Send(make_event<Evt>(method, std::nullopt)))
+ OnError(std::move(error));
+ };
+ } else {
+ return [this, method](const Params ¶ms) {
+ if (llvm::Error error =
+ m_transport.Send(make_event<Evt>(method, toJSON(params))))
+ OnError(std::move(error));
+ };
+ }
+ }
+
+ void Received(const Evt &evt) override {
+ std::scoped_lock<std::recursive_mutex> guard(m_mutex);
+ auto it = m_event_handlers.find(get_method<Evt>(evt));
+ if (it == m_event_handlers.end()) {
+ OnError(llvm::createStringError(
+ llvm::formatv("no handled for event {0}", toJSON(evt))));
+ return;
+ }
+ it->second(evt);
+ }
+
+ void Received(const Req &req) override {
+ ReplyOnce reply(req, &m_transport, this);
+
+ std::scoped_lock<std::recursive_mutex> guard(m_mutex);
+ auto it = m_request_handlers.find(get_method<Req>(req));
+ if (it == m_request_handlers.end()) {
+ reply(make_response<Req, Resp>(
+ req, llvm::createStringError("method not found")));
+ return;
+ }
+
+ it->second(req, std::move(reply));
+ }
+
+ void Received(const Resp &resp) override {
+ std::scoped_lock<std::recursive_mutex> guard(m_mutex);
+ auto it = m_pending_responses.find(get_id<Id, Resp>(resp));
+ if (it == m_pending_responses.end()) {
+ OnError(llvm::createStringError(
+ llvm::formatv("no pending request for {0}", toJSON(resp))));
+ return;
+ }
+
+ it->second(resp);
+ m_pending_responses.erase(it);
+ }
+
+ void OnError(llvm::Error err) override {
+ std::scoped_lock<std::recursive_mutex> guard(m_mutex);
+ if (m_error_handler)
+ m_error_handler(std::move(err));
+ }
+
+ void OnClosed() override {
+ std::scoped_lock<std::recursive_mutex> guard(m_mutex);
+ if (m_disconnect_handler)
+ m_disconnect_handler();
+ }
+
+ private:
+ std::recursive_mutex m_mutex;
+ JSONTransport &m_transport;
+ Id m_seq;
+ std::map<Id, Callback<void(const Resp &)>> m_pending_responses;
+ llvm::StringMap<Callback<void(const Req &, Callback<void(const Resp &)>)>>
+ m_request_handlers;
+ llvm::StringMap<Callback<void(const Evt &)>> m_event_handlers;
+ Callback<void()> m_disconnect_handler;
+ Callback<void(llvm::Error)> m_error_handler;
+ };
};
-/// A JSONTransport will encode and decode messages using JSON.
-template <typename Req, typename Resp, typename Evt>
-class JSONTransport : public Transport<Req, Resp, Evt> {
+/// A IOTransport will encode and decode messages using an IOObject like a
+/// file or a socket.
+template <typename Id, typename Req, typename Resp, typename Evt>
+class IOTransport : public JSONTransport<Id, Req, Resp, Evt> {
public:
- using Transport<Req, Resp, Evt>::Transport;
- using MessageHandler = typename Transport<Req, Resp, Evt>::MessageHandler;
+ using Message = typename JSONTransport<Id, Req, Resp, Evt>::Message;
+ using MessageHandler =
+ typename JSONTransport<Id, Req, Resp, Evt>::MessageHandler;
- JSONTransport(lldb::IOObjectSP in, lldb::IOObjectSP out)
+ IOTransport(lldb::IOObjectSP in, lldb::IOObjectSP out)
: m_in(in), m_out(out) {}
llvm::Error Send(const Evt &evt) override { return Write(evt); }
@@ -127,7 +464,7 @@ class JSONTransport : public Transport<Req, Resp, Evt> {
Status status;
MainLoop::ReadHandleUP read_handle = loop.RegisterReadObject(
m_in,
- std::bind(&JSONTransport::OnRead, this, std::placeholders::_1,
+ std::bind(&IOTransport::OnRead, this, std::placeholders::_1,
std::ref(handler)),
status);
if (status.Fail()) {
@@ -140,7 +477,7 @@ class JSONTransport : public Transport<Req, Resp, Evt> {
/// detail.
static constexpr size_t kReadBufferSize = 1024;
- // FIXME: Write should be protected.
+protected:
llvm::Error Write(const llvm::json::Value &message) {
this->Logv("<-- {0}", message);
std::string output = Encode(message);
@@ -148,7 +485,6 @@ class JSONTransport : public Transport<Req, Resp, Evt> {
return m_out->Write(output.data(), bytes_written).takeError();
}
-protected:
virtual llvm::Expected<std::vector<std::string>> Parse() = 0;
virtual std::string Encode(const llvm::json::Value &message) = 0;
@@ -175,9 +511,8 @@ class JSONTransport : public Transport<Req, Resp, Evt> {
}
for (const std::string &raw_message : *raw_messages) {
- llvm::Expected<typename Transport<Req, Resp, Evt>::Message> message =
- llvm::json::parse<typename Transport<Req, Resp, Evt>::Message>(
- raw_message);
+ llvm::Expected<Message> message =
+ llvm::json::parse<Message>(raw_message);
if (!message) {
handler.OnError(message.takeError());
return;
@@ -202,10 +537,10 @@ class JSONTransport : public Transport<Req, Resp, Evt> {
};
/// A transport class for JSON with a HTTP header.
-template <typename Req, typename Resp, typename Evt>
-class HTTPDelimitedJSONTransport : public JSONTransport<Req, Resp, Evt> {
+template <typename Id, typename Req, typename Resp, typename Evt>
+class HTTPDelimitedJSONTransport : public IOTransport<Id, Req, Resp, Evt> {
public:
- using JSONTransport<Req, Resp, Evt>::JSONTransport;
+ using IOTransport<Id, Req, Resp, Evt>::IOTransport;
protected:
/// Encodes messages based on
@@ -231,8 +566,8 @@ class HTTPDelimitedJSONTransport : public JSONTransport<Req, Resp, Evt> {
for (const llvm::StringRef &header :
llvm::split(headers, kHeaderSeparator)) {
auto [key, value] = header.split(kHeaderFieldSeparator);
- // 'Content-Length' is the only meaningful key at the moment. Others are
- // ignored.
+ // 'Content-Length' is the only meaningful key at the moment. Others
+ // are ignored.
if (!key.equals_insensitive(kHeaderContentLength))
continue;
@@ -269,10 +604,10 @@ class HTTPDelimitedJSONTransport : public JSONTransport<Req, Resp, Evt> {
};
/// A transport class for JSON RPC.
-template <typename Req, typename Resp, typename Evt>
-class JSONRPCTransport : public JSONTransport<Req, Resp, Evt> {
+template <typename Id, typename Req, typename Resp, typename Evt>
+class JSONRPCTransport : public IOTransport<Id, Req, Resp, Evt> {
public:
- using JSONTransport<Req, Resp, Evt>::JSONTransport;
+ using IOTransport<Id, Req, Resp, Evt>::IOTransport;
protected:
std::string Encode(const llvm::json::Value &message) override {
diff --git a/lldb/include/lldb/Protocol/MCP/Protocol.h b/lldb/include/lldb/Protocol/MCP/Protocol.h
index 6e1ffcbe1f3e3..1e0816110b80a 100644
--- a/lldb/include/lldb/Protocol/MCP/Protocol.h
+++ b/lldb/include/lldb/Protocol/MCP/Protocol.h
@@ -14,6 +14,7 @@
#ifndef LLDB_PROTOCOL_MCP_PROTOCOL_H
#define LLDB_PROTOCOL_MCP_PROTOCOL_H
+#include "llvm/ADT/StringRef.h"
#include "llvm/Support/JSON.h"
#include <optional>
#include <string>
@@ -324,4 +325,11 @@ bool fromJSON(const llvm::json::Value &, CallToolResult &, llvm::json::Path);
} // namespace lldb_pr...
[truncated]
|
This adds a new Binding helper class to allow mapping of incoming and outgoing requests / events to specific handlers. This should make it easier to create new protocol implementations and allow us to create a relay in the lldb-mcp binary.
| class Binder; | ||
| using BinderUP = std::unique_ptr<Binder>; | ||
|
|
||
| /// Binder collects a table of functions that handle calls. |
There was a problem hiding this comment.
I think this new class warrants a bit more documentation. IIUC, this is basically a forwarded of messages, which allows you to register a handler by name. By defining message types we can get that automatically out of the messages, and then have it call the right callback?
Basically, this hoist the logic we have both in MCP and DAP into the transport class. Which leads to my next question: does this need to be tied to JSONTransport? Can this be built on top of an arbitrary transport?
There was a problem hiding this comment.
By defining message types we can get that automatically out of the messages, and then have it call the right callback?
Yea, for example, from the Server.cpp
MCPTransport::BinderUP Server::Bind(MCPTransport &transport) {
MCPTransport::BinderUP binder =
std::make_unique<MCPTransport::Binder>(transport);
binder->bind<InitializeResult, InitializeParams>(
"initialize", &Server::InitializeHandler, this);
binder->bind<ListToolsResult, void>("tools/list", &Server::ToolsListHandler,
this);
binder->bind<CallToolResult, CallToolParams>("tools/call",
&Server::ToolsCallHandler, this);
binder->bind<ListResourcesResult, void>("resources/list",
&Server::ResourcesListHandler, this);
binder->bind<ReadResourceResult, ReadResourceParams>(
"resources/read", &Server::ResourcesReadHandler, this);
binder->bind<void>("notifications/initialized",
[this]() { Log("MCP initialization complete"); });
return binder;
}
The bind method setups the serialization of the Params and Results and wraps them into the Request and Response types.
This means the server doesn't need to implement its own method dispatching mechanism, that can be handled by the Binder and Server can be simplified a bit. You can also see an example in JSONTransportTest.cpp that does this simple binding of a incoming call, outgoing call, incoming event and outgoing event.
does this need to be tied to JSONTransport? Can this be built on top of an arbitrary transport?
Within each bind call we do assume that Params and Result types are all JSON serializable.
That could be pulled apart a bit more though if we wanted to remove the JSON requirement. That could become a parameter to the template I suppose.
There was a problem hiding this comment.
Also, another benefit of the binder not being part of the Server implementation here specifically is that the Server can choose if it wants to handle a single client or multiple clients per instance. We can bind the methods with additional context parameters to make the server independent of the transport.
ashgti
force-pushed
the
lldb-jsontransport-binder
branch
from
4acc368 to
d861156
Compare
Moved the Binder below the rest of the transport classes and moved some of the implementations below the class definition to improve readability of the Binder interface without seeing a lot of template details.
| /// Extracts the parameters from a request or event. | ||
| template <typename T> llvm::json::Value GetParams(const T &); | ||
|
|
||
| /// Binder collects a table of functions that handle calls. |
There was a problem hiding this comment.
I apologize for the bike-shedding, but maybe it would be helpful to describe this in terms of JSONRPC. Something like, the binder registers functions/callback for JSONRPC remote method calls. The JSONRPC transport takes care of parsing incoming calls and serializing responses.
There was a problem hiding this comment.
I wasn't super happy with these free template functions being required for the Binder to function.
I instead merged this into a ProtocolDescriptor that makes this into a single object at least. Let me know what you think.
The JSONTransport having to carry around 4 template parameters was a lot of repeated information. To try to simplify this, I created a 'concept' for `ProtocolDescriptor` that describes the single type definition used to describes a protocol. The `BindingBuilder` concept is an extension on `ProtocolDescriptor` that has additional helpers for using the Binder. This helps relate the concepts together into a single place, which I find helpful. Previously, there were a few free standing template functions that needed to be implemented for the Binder to work.
| /// A function to send an outgoing event. | ||
| template <typename P> using OutgoingEvent = typename detail::event_t<P>::type; | ||
|
|
||
| // FIXME: With c++20, we should use this concept: |
There was a problem hiding this comment.
I find commented out code really annoying and I strongly prefer putting code behind #if 0. Or even better, if it compiles with C++20 support, we could gate this by #if __cplusplus >= 202002L.
|
So, this is a bit of a stylistic question but I wasn't super happy with how I needed to pass template 4 parameters around and I had a few free template functions for the Binder class to work correctly. I made an attempt at consolidating that into a Here is what they look like as concepts, which only work when compiling in c++20 mode. I've left them in the JSONTransport.h header but commented out. Do you have a preference between passing 4 template parameters and the free template functions vs the single descriptor type? |
|
@JDevlieghere Any thoughts on my style question about the code structure? |
|
I like the single template argument and using its dependent types. I don't know enough about C++20 concepts to have an opinion on that part, but if your proposed approach happens to bring the two implementations closer together, then that seems like a nice to have. |
JDevlieghere
approved these changes
Oct 2, 2025
|
Please fix tests on Windows. |
|
Taking a look. |
|
I'm seeing compilation errors with gcc 11: |
Or that's just the assert going off and using something in CRT to do so. This could be coming from https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getfiletype These are test cases, so it would make some sense if one of the streams was not a pipe but some premade string or text file. |
I am seeing the same compilation errors. |
|
I can update the test names to not match the using definition. |
|
#162693 should address gcc11 I think. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a new Binding helper class to allow mapping of incoming and outgoing requests / events to specific handlers.
This should make it easier to create new protocol implementations and allow us to create a relay in the lldb-mcp binary.