-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[lldb-dap] Updating RequestHandler to encode/decode arguments and response. #130090
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
72956b9
to
36d0fec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I like the overall direction this is going
lldb/tools/lldb-dap/Protocol.h
Outdated
// }, | ||
// "required": [ "sourceReference" ] | ||
// }, | ||
struct SourceArguments { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should change how we structure our files.
IMO, we should have SourceArguments
, SourceResponse
and SourceRequestHandler
all in Handlers/SourceRequestHandler.{h,cpp}
. That way, we have all the SourceRequest
-related things in one place.
Not sure where Source
should go. If it's only used by the SourceRequestHandler
, I would also put it into Handlers/SourceRequestHandler.{h,cpp}
, otherwise we might move it to ProtocolTypes.{h,cpp}
or even ProtocolTypes/Source.{h,cpp}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise we might move it to ProtocolTypes.{h,cpp} or even ProtocolTypes/Source.{h,cpp}
and maybe move the existing Protocol.{h,cpp}
to BaseProtocol.{h,cpp}
? To keep the base protocol separated from the other request / response / event types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally like the protocol being isolated into its own namespace and its own file.
There are actually a few examples of this already in llvm, e.g. clang-tools-extra/clangd/Protocol.h or mlir/include/mlir/Tools/lsp-server-support/Protocol.h.
This is nice because we can easily look in the file for other types defined in the protocol and add new types as we need them. For example, you mentioned the Source
type, which is used in a few events and requests.
The DAP spec already groups at a high level into Base Protocol
, Events
, Requests
, Reverse Requests
and Types
, which I was roughly following in this file using // MARK: <category>
headings between sections.
I only left the description of the SourceRequest
next to the handler for reference for the implementation.
I'm fine with changing how we structure things, but I liked having the distinction between lldb_dap::protocol
for the underlying protocol, lldb_dap
for our intermediate components and lldb
/lldb_private
for the SB API layer of the debugger itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with changing how we structure things, but I liked having the distinction between lldb_dap::protocol for the underlying protocol, lldb_dap for our intermediate components and lldb/lldb_private for the SB API layer of the debugger itself.
Indeed, you brought up a couple of good arguments for keeping the protocol separate. I am undecided. Let's see if anyone else jumps in and also has additional arguments in either direction. Otherwise, I would be fine with either way.
The DAP spec already groups at a high level into Base Protocol, Events, Requests, Reverse Requests and Types, which I was roughly following in this file using // MARK: headings between sections.
Even if we don't move *Args
, *Request
and *Response
types to the corresponding *RequestHandler
, I think we should still separate the base protocol from the remaining protocol types. I would prefer separate files over // MARK: <category>
comments for that purpose, i.e. a Protocol.{h,cpp}
(containing the events, request and response types) and a separate BaseProtocol.{h,cpp}
(containing the base protocol)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JDevlieghere Any thoughts on the organization of these types?
I was keeping things simple by putting the types in a single place, but we don't strictly need to do that.
We could have ProtocolBase.h
, ProtocolTypes.h
, ProtocolRequests.h
, ProtocolEvents.h
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see an argument for either. If it were up to me, I'd go with the different Protocol*
files in a Protocol
subdirectory. My (entirely subjective) reasoning is that the protocol should match the spec, rather than how it's used in our implementation and therefore it warrants standing on its own. Things were different when we didn't have the abstraction and type safety offered by the Protocol class and we had to consult the spec to decode a message, but now that's not a concern anymore if you're using an IDE or editor with code completion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split up the Protocol.{h,cpp}
> Protocol/Protocol{Base,Requests,Types}.{h,cpp}
.
…ponse. This is a work in progress refactor to add explicit types instead of generic 'llvm::json::Value' types to the DAP protocol. This updates RequestHandler to have take the type of the arguments and response body for serialization for requests. The 'source' request is updated to show how the new flow works.
…stead extracting the comments to their c++ repr.
36d0fec
to
9908931
Compare
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesThis is a work in progress refactor to add explicit types instead of This updates RequestHandler to have take the type of the arguments and The 'source' request is updated to show how the new flow works. This is built on top of #130026 Patch is 43.12 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/130090.diff 8 Files Affected:
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 4080e2c211035..9d42af2d7091f 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -8,10 +8,12 @@
#include "DAP.h"
#include "DAPLog.h"
+#include "Handler/RequestHandler.h"
#include "Handler/ResponseHandler.h"
#include "JSONUtils.h"
#include "LLDBUtils.h"
#include "OutputRedirector.h"
+#include "Protocol.h"
#include "Transport.h"
#include "lldb/API/SBBreakpoint.h"
#include "lldb/API/SBCommandInterpreter.h"
@@ -25,6 +27,7 @@
#include "lldb/lldb-defines.h"
#include "lldb/lldb-enumerations.h"
#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
@@ -41,6 +44,7 @@
#include <fstream>
#include <memory>
#include <mutex>
+#include <string>
#include <utility>
#if defined(_WIN32)
@@ -663,31 +667,23 @@ void DAP::SetTarget(const lldb::SBTarget target) {
}
bool DAP::HandleObject(const protocol::Message &M) {
- // FIXME: Directly handle `Message` instead of serializing to JSON.
- llvm::json::Value v = toJSON(M);
- llvm::json::Object object = *v.getAsObject();
- const auto packet_type = GetString(object, "type");
- if (packet_type == "request") {
- const auto command = GetString(object, "command");
-
- auto new_handler_pos = request_handlers.find(command);
+ if (const auto *req = std::get_if<protocol::Request>(&M)) {
+ auto new_handler_pos = request_handlers.find(req->command);
if (new_handler_pos != request_handlers.end()) {
- (*new_handler_pos->second)(object);
+ (*new_handler_pos->second)(*req);
return true; // Success
}
DAP_LOG(log, "({0}) error: unhandled command '{1}'",
- transport.GetClientName(), command);
+ transport.GetClientName(), req->command);
return false; // Fail
}
- if (packet_type == "response") {
- auto id = GetInteger<int64_t>(object, "request_seq").value_or(0);
-
+ if (const auto *resp = std::get_if<protocol::Response>(&M)) {
std::unique_ptr<ResponseHandler> response_handler;
{
std::lock_guard<std::mutex> locker(call_mutex);
- auto inflight = inflight_reverse_requests.find(id);
+ auto inflight = inflight_reverse_requests.find(resp->request_seq);
if (inflight != inflight_reverse_requests.end()) {
response_handler = std::move(inflight->second);
inflight_reverse_requests.erase(inflight);
@@ -695,19 +691,31 @@ bool DAP::HandleObject(const protocol::Message &M) {
}
if (!response_handler)
- response_handler = std::make_unique<UnknownResponseHandler>("", id);
+ response_handler =
+ std::make_unique<UnknownResponseHandler>("", resp->request_seq);
// Result should be given, use null if not.
- if (GetBoolean(object, "success").value_or(false)) {
- llvm::json::Value Result = nullptr;
- if (auto *B = object.get("body"))
- Result = std::move(*B);
- (*response_handler)(Result);
+ if (resp->success) {
+ (*response_handler)(resp->rawBody);
} else {
- llvm::StringRef message = GetString(object, "message");
- if (message.empty()) {
- message = "Unknown error, response failed";
+ std::string message = "Unknown error, response failed";
+ if (resp->message) {
+ message = std::visit(
+ llvm::makeVisitor(
+ [](const std::string &message) -> std::string {
+ return message;
+ },
+ [](const protocol::Response::Message &message) -> std::string {
+ switch (message) {
+ case protocol::Response::Message::cancelled:
+ return "cancelled";
+ case protocol::Response::Message::notStopped:
+ return "notStopped";
+ }
+ }),
+ *resp->message);
}
+
(*response_handler)(llvm::createStringError(
std::error_code(-1, std::generic_category()), message));
}
@@ -715,6 +723,9 @@ bool DAP::HandleObject(const protocol::Message &M) {
return true;
}
+ if (log)
+ *log << "Unsupported protocol message" << std::endl;
+
return false;
}
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index db3473b7c7027..f6b63e4a3e0bc 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -12,8 +12,6 @@
#include "DAPForward.h"
#include "ExceptionBreakpoint.h"
#include "FunctionBreakpoint.h"
-#include "Handler/RequestHandler.h"
-#include "Handler/ResponseHandler.h"
#include "InstructionBreakpoint.h"
#include "OutputRedirector.h"
#include "ProgressEvent.h"
@@ -187,7 +185,7 @@ struct DAP {
// the old process here so we can detect this case and keep running.
lldb::pid_t restarting_process_id;
bool configuration_done_sent;
- llvm::StringMap<std::unique_ptr<RequestHandler>> request_handlers;
+ llvm::StringMap<std::unique_ptr<BaseRequestHandler>> request_handlers;
bool waiting_for_run_in_terminal;
ProgressEventReporter progress_event_reporter;
// Keep track of the last stop thread index IDs as threads won't go away
diff --git a/lldb/tools/lldb-dap/DAPForward.h b/lldb/tools/lldb-dap/DAPForward.h
index 0196d83dcd6a9..667aef23abd0f 100644
--- a/lldb/tools/lldb-dap/DAPForward.h
+++ b/lldb/tools/lldb-dap/DAPForward.h
@@ -19,6 +19,8 @@ struct SourceBreakpoint;
struct Watchpoint;
struct InstructionBreakpoint;
struct DAP;
+class BaseRequestHandler;
+class ResponseHandler;
} // namespace lldb_dap
namespace lldb {
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
index e43fa36d25e3f..13cf166e200e8 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
@@ -6,8 +6,9 @@
//
//===----------------------------------------------------------------------===//
-#include "RequestHandler.h"
+#include "Handler/RequestHandler.h"
#include "DAP.h"
+#include "Handler/ResponseHandler.h"
#include "JSONUtils.h"
#include "LLDBUtils.h"
#include "RunInTerminal.h"
@@ -45,7 +46,7 @@ static uint32_t SetLaunchFlag(uint32_t flags, const llvm::json::Object *obj,
// Both attach and launch take either a sourcePath or a sourceMap
// argument (or neither), from which we need to set the target.source-map.
-void RequestHandler::SetSourceMapFromArguments(
+void BaseRequestHandler::SetSourceMapFromArguments(
const llvm::json::Object &arguments) const {
const char *sourceMapHelp =
"source must be be an array of two-element arrays, "
@@ -159,7 +160,7 @@ static llvm::Error RunInTerminal(DAP &dap,
}
lldb::SBError
-RequestHandler::LaunchProcess(const llvm::json::Object &request) const {
+BaseRequestHandler::LaunchProcess(const llvm::json::Object &request) const {
lldb::SBError error;
const auto *arguments = request.getObject("arguments");
auto launchCommands = GetStrings(arguments, "launchCommands");
@@ -228,13 +229,13 @@ RequestHandler::LaunchProcess(const llvm::json::Object &request) const {
return error;
}
-void RequestHandler::PrintWelcomeMessage() const {
+void BaseRequestHandler::PrintWelcomeMessage() const {
#ifdef LLDB_DAP_WELCOME_MESSAGE
dap.SendOutput(OutputType::Console, LLDB_DAP_WELCOME_MESSAGE);
#endif
}
-bool RequestHandler::HasInstructionGranularity(
+bool BaseRequestHandler::HasInstructionGranularity(
const llvm::json::Object &arguments) const {
if (std::optional<llvm::StringRef> value = arguments.getString("granularity"))
return value == "instruction";
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index b44367518bcb9..44a1aa4cefcde 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -9,25 +9,36 @@
#ifndef LLDB_TOOLS_LLDB_DAP_HANDLER_HANDLER_H
#define LLDB_TOOLS_LLDB_DAP_HANDLER_HANDLER_H
+#include "DAP.h"
+#include "Protocol.h"
#include "lldb/API/SBError.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/JSON.h"
namespace lldb_dap {
struct DAP;
-class RequestHandler {
+/// Base class for request handlers. Do not extend this directly: Extend
+/// the RequestHandler template subclass instead.
+class BaseRequestHandler {
public:
- RequestHandler(DAP &dap) : dap(dap) {}
+ BaseRequestHandler(DAP &dap) : dap(dap) {}
- /// RequestHandler are not copyable.
+ /// BaseRequestHandler are not copyable.
/// @{
- RequestHandler(const RequestHandler &) = delete;
- RequestHandler &operator=(const RequestHandler &) = delete;
+ BaseRequestHandler(const BaseRequestHandler &) = delete;
+ BaseRequestHandler &operator=(const BaseRequestHandler &) = delete;
/// @}
- virtual ~RequestHandler() = default;
+ virtual ~BaseRequestHandler() = default;
+ virtual void operator()(const protocol::Request &request) const {
+ auto req = toJSON(request);
+ (*this)(*req.getAsObject());
+ }
+
+ /// FIXME: Migrate callers to typed RequestHandler for improved type handling.
virtual void operator()(const llvm::json::Object &request) const = 0;
protected:
@@ -57,235 +68,288 @@ class RequestHandler {
DAP &dap;
};
-class AttachRequestHandler : public RequestHandler {
-public:
- using RequestHandler::RequestHandler;
+/// Base class for handling DAP requests. Handlers should declare their
+/// arguments and response body types like:
+///
+/// class MyRequestHandler : public RequestHandler<Arguments, ResponseBody> {
+/// ....
+/// };
+template <typename Args, typename Body>
+class RequestHandler : public BaseRequestHandler {
+ using BaseRequestHandler::BaseRequestHandler;
+
+ void operator()(const llvm::json::Object &request) const override {
+ /* no-op, the other overload handles json coding. */
+ }
+
+ void operator()(const protocol::Request &request) const override {
+ protocol::Response response;
+ response.request_seq = request.seq;
+ response.command = request.command;
+ Args arguments;
+ llvm::json::Path::Root root;
+ if (request.rawArguments &&
+ !fromJSON(request.rawArguments, arguments, root)) {
+ std::string parseFailure;
+ llvm::raw_string_ostream OS(parseFailure);
+ root.printErrorContext(request.rawArguments, OS);
+ response.success = false;
+ response.message = parseFailure;
+ dap.SendJSON(std::move(response));
+ return;
+ }
+
+ auto ResponseBody = Run(arguments);
+ // FIXME: Add a dedicated DAPError for enhanced errors that are
+ // user-visibile.
+ if (auto Err = ResponseBody.takeError()) {
+ response.success = false;
+ // FIXME: Build ErrorMessage based on error details instead of using the
+ // 'message' field.
+ response.message = llvm::toString(std::move(Err));
+ } else {
+ response.success = true;
+ response.rawBody = std::move(*ResponseBody);
+ }
+
+ dap.SendJSON(std::move(response));
+ };
+
+ virtual llvm::Expected<Body> Run(const Args &) const = 0;
+};
+
+class AttachRequestHandler : public BaseRequestHandler {
+public:
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "attach"; }
void operator()(const llvm::json::Object &request) const override;
};
-class BreakpointLocationsRequestHandler : public RequestHandler {
+class BreakpointLocationsRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "breakpointLocations"; }
void operator()(const llvm::json::Object &request) const override;
};
-class CompletionsRequestHandler : public RequestHandler {
+class CompletionsRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "completions"; }
void operator()(const llvm::json::Object &request) const override;
};
-class ContinueRequestHandler : public RequestHandler {
+class ContinueRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "continue"; }
void operator()(const llvm::json::Object &request) const override;
};
-class ConfigurationDoneRequestHandler : public RequestHandler {
+class ConfigurationDoneRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "configurationDone"; }
void operator()(const llvm::json::Object &request) const override;
};
-class DisconnectRequestHandler : public RequestHandler {
+class DisconnectRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "disconnect"; }
void operator()(const llvm::json::Object &request) const override;
};
-class EvaluateRequestHandler : public RequestHandler {
+class EvaluateRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "evaluate"; }
void operator()(const llvm::json::Object &request) const override;
};
-class ExceptionInfoRequestHandler : public RequestHandler {
+class ExceptionInfoRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "exceptionInfo"; }
void operator()(const llvm::json::Object &request) const override;
};
-class InitializeRequestHandler : public RequestHandler {
+class InitializeRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "initialize"; }
void operator()(const llvm::json::Object &request) const override;
};
-class LaunchRequestHandler : public RequestHandler {
+class LaunchRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "launch"; }
void operator()(const llvm::json::Object &request) const override;
};
-class RestartRequestHandler : public RequestHandler {
+class RestartRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "restart"; }
void operator()(const llvm::json::Object &request) const override;
};
-class NextRequestHandler : public RequestHandler {
+class NextRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "next"; }
void operator()(const llvm::json::Object &request) const override;
};
-class StepInRequestHandler : public RequestHandler {
+class StepInRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "stepIn"; }
void operator()(const llvm::json::Object &request) const override;
};
-class StepInTargetsRequestHandler : public RequestHandler {
+class StepInTargetsRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "stepInTargets"; }
void operator()(const llvm::json::Object &request) const override;
};
-class StepOutRequestHandler : public RequestHandler {
+class StepOutRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "stepOut"; }
void operator()(const llvm::json::Object &request) const override;
};
-class SetBreakpointsRequestHandler : public RequestHandler {
+class SetBreakpointsRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "setBreakpoints"; }
void operator()(const llvm::json::Object &request) const override;
};
-class SetExceptionBreakpointsRequestHandler : public RequestHandler {
+class SetExceptionBreakpointsRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "setExceptionBreakpoints"; }
void operator()(const llvm::json::Object &request) const override;
};
-class SetFunctionBreakpointsRequestHandler : public RequestHandler {
+class SetFunctionBreakpointsRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "setFunctionBreakpoints"; }
void operator()(const llvm::json::Object &request) const override;
};
-class DataBreakpointInfoRequestHandler : public RequestHandler {
+class DataBreakpointInfoRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "dataBreakpointInfo"; }
void operator()(const llvm::json::Object &request) const override;
};
-class SetDataBreakpointsRequestHandler : public RequestHandler {
+class SetDataBreakpointsRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "setDataBreakpoints"; }
void operator()(const llvm::json::Object &request) const override;
};
-class SetInstructionBreakpointsRequestHandler : public RequestHandler {
+class SetInstructionBreakpointsRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() {
return "setInstructionBreakpoints";
}
void operator()(const llvm::json::Object &request) const override;
};
-class CompileUnitsRequestHandler : public RequestHandler {
+class CompileUnitsRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "compileUnits"; }
void operator()(const llvm::json::Object &request) const override;
};
-class ModulesRequestHandler : public RequestHandler {
+class ModulesRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "modules"; }
void operator()(const llvm::json::Object &request) const override;
};
-class PauseRequestHandler : public RequestHandler {
+class PauseRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "pause"; }
void operator()(const llvm::json::Object &request) const override;
};
-class ScopesRequestHandler : public RequestHandler {
+class ScopesRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseRequestHandler;
static llvm::StringLiteral getCommand() { return "scopes"; }
void operator()(const llvm::json::Object &request) const override;
};
-class SetVariableRequestHandler : public RequestHandler {
+class SetVariableRequestHandler : public BaseRequestHandler {
public:
- using RequestHandler::RequestHandler;
+ using BaseRequestHandler::BaseReque...
[truncated]
|
What is your overall plan for this PR? I assume the first step is to get buy-in from all reviewers into the new structure for our JSON types. But what comes after? Are you planning to update this PR to cover all JSON types / requests / responses? Are you planning to land this PR for the (Both approaches would be fine by me. Just trying to understand the approach, so I know when it's time to hit the "Approve" button 😉) |
Co-authored-by: Adrian Vogelsgesang <[email protected]>
…ssages without a response body. To validate both of these changes I also migrated the 'disconnect' request to have new types, since it has an `optional<DisconnectArguments>` and a `VoidResponse` body.
I was thinking of doing this more incrementally. I was planning to build on this for handling the 'cancel' request first but then I'd like to follow up with requests to use the new explicit types for handling arguments and responses. |
…`DAP::Disconnect` method a little more ergnomic.
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small drive-by. This generally looks good to me, but I'll leave the approval to someone else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…refix. The types should be enough to tell that theyre not specific values and this helps with the naming consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo comments
This is a work in progress refactor to add explicit types instead of
generic 'llvm::json::Value' types to the DAP protocol.
This updates RequestHandler to have take the type of the arguments and
response body for serialization for requests.
The 'source' and 'disconnect' request is updated to show how the new flow
works and includes serialization handling for optional arguments and 'void'
responses.
This is built on top of #130026