[lldb-dap] Creating a dedicated Transport class for DAP communication. #128972
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
…ocol communication. This removes the previous 'OutputStream' and 'InputStream' objects and the new 'Transport' class takes on the responsibility of seriliaizing and encoding messages. The new Protocol.h file has a dedicated namespace for POD style structures to represent the Debug Adapter Protocol messages.
ashgti
force-pushed
the
lldb-dap-transport
branch
from
cc70fea to
397c479
Compare
Member
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesThis removes the previous 'OutputStream' and 'InputStream' objects and the new 'Transport' class takes on the responsibility of serializing and encoding messages. The new Protocol.h file has a dedicated namespace for POD style structures to represent the Debug Adapter Protocol messages. This should help unblock supporting 'cancel' requests by allowing us to have more detailed type information. Patch is 34.88 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/128972.diff 12 Files Affected:
diff --git a/lldb/tools/lldb-dap/CMakeLists.txt b/lldb/tools/lldb-dap/CMakeLists.txt
index 8b3c520ec4360..cba20a53edcb3 100644
--- a/lldb/tools/lldb-dap/CMakeLists.txt
+++ b/lldb/tools/lldb-dap/CMakeLists.txt
@@ -23,18 +23,20 @@ add_lldb_tool(lldb-dap
Breakpoint.cpp
BreakpointBase.cpp
DAP.cpp
+ DAPLog.cpp
EventHelper.cpp
ExceptionBreakpoint.cpp
FifoFiles.cpp
FunctionBreakpoint.cpp
InstructionBreakpoint.cpp
- IOStream.cpp
JSONUtils.cpp
LLDBUtils.cpp
OutputRedirector.cpp
ProgressEvent.cpp
RunInTerminal.cpp
SourceBreakpoint.cpp
+ Transport.cpp
+ Protocol.cpp
Watchpoint.cpp
Handler/ResponseHandler.cpp
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index cd53e2aca3fb6..e23d9f4e910cc 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -7,10 +7,12 @@
//===----------------------------------------------------------------------===//
#include "DAP.h"
+#include "DAPLog.h"
#include "Handler/ResponseHandler.h"
#include "JSONUtils.h"
#include "LLDBUtils.h"
#include "OutputRedirector.h"
+#include "Protocol.h"
#include "lldb/API/SBBreakpoint.h"
#include "lldb/API/SBCommandInterpreter.h"
#include "lldb/API/SBCommandReturnObject.h"
@@ -29,6 +31,7 @@
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
#include "llvm/Support/raw_ostream.h"
#include <algorithm>
#include <cassert>
@@ -50,6 +53,7 @@
#endif
using namespace lldb_dap;
+using namespace lldb_private;
namespace {
#ifdef _WIN32
@@ -61,11 +65,11 @@ const char DEV_NULL[] = "/dev/null";
namespace lldb_dap {
-DAP::DAP(std::string name, llvm::StringRef path, std::ofstream *log,
+DAP::DAP(llvm::StringRef name, llvm::StringRef path, std::ofstream *log,
lldb::IOObjectSP input, lldb::IOObjectSP output, ReplMode repl_mode,
std::vector<std::string> pre_init_commands)
- : name(std::move(name)), debug_adaptor_path(path), log(log),
- input(std::move(input)), output(std::move(output)),
+ : name(name), debug_adaptor_path(path), log(log),
+ transport(name, std::move(input), std::move(output)),
broadcaster("lldb-dap"), exception_breakpoints(),
pre_init_commands(std::move(pre_init_commands)),
focus_tid(LLDB_INVALID_THREAD_ID), stop_at_entry(false), is_attach(false),
@@ -237,65 +241,22 @@ void DAP::StopEventHandlers() {
}
}
-// Send the JSON in "json_str" to the "out" stream. Correctly send the
-// "Content-Length:" field followed by the length, followed by the raw
-// JSON bytes.
-void DAP::SendJSON(const std::string &json_str) {
- output.write_full("Content-Length: ");
- output.write_full(llvm::utostr(json_str.size()));
- output.write_full("\r\n\r\n");
- output.write_full(json_str);
-}
-
// Serialize the JSON value into a string and send the JSON packet to
// the "out" stream.
void DAP::SendJSON(const llvm::json::Value &json) {
- std::string json_str;
- llvm::raw_string_ostream strm(json_str);
- strm << json;
- static std::mutex mutex;
- std::lock_guard<std::mutex> locker(mutex);
- SendJSON(json_str);
-
- if (log) {
- auto now = std::chrono::duration<double>(
- std::chrono::system_clock::now().time_since_epoch());
- *log << llvm::formatv("{0:f9} {1} <-- ", now.count(), name).str()
- << std::endl
- << "Content-Length: " << json_str.size() << "\r\n\r\n"
- << llvm::formatv("{0:2}", json).str() << std::endl;
- }
-}
-
-// Read a JSON packet from the "in" stream.
-std::string DAP::ReadJSON() {
- std::string length_str;
- std::string json_str;
- int length;
-
- if (!input.read_expected(log, "Content-Length: "))
- return json_str;
-
- if (!input.read_line(log, length_str))
- return json_str;
-
- if (!llvm::to_integer(length_str, length))
- return json_str;
-
- if (!input.read_expected(log, "\r\n"))
- return json_str;
-
- if (!input.read_full(log, length, json_str))
- return json_str;
-
- if (log) {
- auto now = std::chrono::duration<double>(
- std::chrono::system_clock::now().time_since_epoch());
- *log << llvm::formatv("{0:f9} {1} --> ", now.count(), name).str()
- << std::endl
- << "Content-Length: " << length << "\r\n\r\n";
+ protocol::ProtocolMessage M;
+ llvm::json::Path::Root root;
+ if (!protocol::fromJSON(json, M, root)) {
+ if (log) {
+ std::string error;
+ llvm::raw_string_ostream OS(error);
+ root.printErrorContext(json, OS);
+ *log << "encoding failure: " << error << "\n";
+ }
}
- return json_str;
+ auto status = transport.Write(M);
+ if (status.Fail() && log)
+ *log << "transport failure: " << status.AsCString() << "\n";
}
// "OutputEvent": {
@@ -720,40 +681,13 @@ void DAP::SetTarget(const lldb::SBTarget target) {
}
}
-PacketStatus DAP::GetNextObject(llvm::json::Object &object) {
- std::string json = ReadJSON();
- if (json.empty())
- return PacketStatus::EndOfFile;
-
- llvm::StringRef json_sref(json);
- llvm::Expected<llvm::json::Value> json_value = llvm::json::parse(json_sref);
- if (!json_value) {
- auto error = json_value.takeError();
- if (log) {
- std::string error_str;
- llvm::raw_string_ostream strm(error_str);
- strm << error;
- *log << "error: failed to parse JSON: " << error_str << std::endl
- << json << std::endl;
- }
- return PacketStatus::JSONMalformed;
- }
-
- if (log) {
- *log << llvm::formatv("{0:2}", *json_value).str() << std::endl;
- }
-
- llvm::json::Object *object_ptr = json_value->getAsObject();
- if (!object_ptr) {
- if (log)
- *log << "error: json packet isn't a object" << std::endl;
- return PacketStatus::JSONNotObject;
- }
- object = *object_ptr;
- return PacketStatus::Success;
+llvm::Expected<protocol::ProtocolMessage> DAP::GetNextObject() {
+ return transport.Read();
}
-bool DAP::HandleObject(const llvm::json::Object &object) {
+bool DAP::HandleObject(const protocol::ProtocolMessage &M) {
+ 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");
@@ -764,9 +698,8 @@ bool DAP::HandleObject(const llvm::json::Object &object) {
return true; // Success
}
- if (log)
- *log << "error: unhandled command \"" << command.data() << "\""
- << std::endl;
+ LLDB_LOG(GetLog(DAPLog::Protocol), "Unhandled command {0}", command);
+
return false; // Fail
}
@@ -805,6 +738,8 @@ bool DAP::HandleObject(const llvm::json::Object &object) {
return true;
}
+ LLDB_LOG(GetLog(DAPLog::Protocol), "Unsupported protocol message");
+
return false;
}
@@ -858,19 +793,13 @@ lldb::SBError DAP::Disconnect(bool terminateDebuggee) {
llvm::Error DAP::Loop() {
auto cleanup = llvm::make_scope_exit([this]() { StopEventHandlers(); });
while (!disconnecting) {
- llvm::json::Object object;
- lldb_dap::PacketStatus status = GetNextObject(object);
-
- if (status == lldb_dap::PacketStatus::EndOfFile) {
- break;
- }
+ auto next = GetNextObject();
- if (status != lldb_dap::PacketStatus::Success) {
- return llvm::createStringError(llvm::inconvertibleErrorCode(),
- "failed to send packet");
+ if (!next) {
+ return next.takeError();
}
- if (!HandleObject(object)) {
+ if (!HandleObject(*next)) {
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"unhandled packet");
}
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index a7c7e5d9bbc19..aa86064e188a9 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -14,11 +14,12 @@
#include "FunctionBreakpoint.h"
#include "Handler/RequestHandler.h"
#include "Handler/ResponseHandler.h"
-#include "IOStream.h"
#include "InstructionBreakpoint.h"
#include "OutputRedirector.h"
#include "ProgressEvent.h"
+#include "Protocol.h"
#include "SourceBreakpoint.h"
+#include "Transport.h"
#include "lldb/API/SBBroadcaster.h"
#include "lldb/API/SBCommandInterpreter.h"
#include "lldb/API/SBDebugger.h"
@@ -39,6 +40,7 @@
#include "llvm/Support/Error.h"
#include "llvm/Support/JSON.h"
#include "llvm/Support/Threading.h"
+#include <fstream>
#include <map>
#include <memory>
#include <mutex>
@@ -145,11 +147,10 @@ struct SendEventRequestHandler : public lldb::SBCommandPluginInterface {
};
struct DAP {
- std::string name;
+ llvm::StringRef name;
llvm::StringRef debug_adaptor_path;
std::ofstream *log;
- InputStream input;
- OutputStream output;
+ Transport transport;
lldb::SBFile in;
OutputRedirector out;
OutputRedirector err;
@@ -210,7 +211,7 @@ struct DAP {
// will contain that expression.
std::string last_nonempty_var_expression;
- DAP(std::string name, llvm::StringRef path, std::ofstream *log,
+ DAP(llvm::StringRef name, llvm::StringRef path, std::ofstream *log,
lldb::IOObjectSP input, lldb::IOObjectSP output, ReplMode repl_mode,
std::vector<std::string> pre_init_commands);
~DAP();
@@ -233,8 +234,6 @@ struct DAP {
// the "out" stream.
void SendJSON(const llvm::json::Value &json);
- std::string ReadJSON();
-
void SendOutput(OutputType o, const llvm::StringRef output);
void SendProgressEvent(uint64_t progress_id, const char *message,
@@ -307,8 +306,8 @@ struct DAP {
/// listeing for its breakpoint events.
void SetTarget(const lldb::SBTarget target);
- PacketStatus GetNextObject(llvm::json::Object &object);
- bool HandleObject(const llvm::json::Object &object);
+ llvm::Expected<protocol::ProtocolMessage> GetNextObject();
+ bool HandleObject(const protocol::ProtocolMessage &);
/// Disconnect the DAP session.
lldb::SBError Disconnect();
diff --git a/lldb/tools/lldb-dap/DAPLog.cpp b/lldb/tools/lldb-dap/DAPLog.cpp
new file mode 100644
index 0000000000000..73d256098d69d
--- /dev/null
+++ b/lldb/tools/lldb-dap/DAPLog.cpp
@@ -0,0 +1,20 @@
+#include "DAPLog.h"
+
+using namespace lldb_private;
+using namespace lldb_dap;
+
+static constexpr Log::Category g_categories[] = {
+ {{"transport"}, {"log DAP transport"}, DAPLog::Transport},
+ {{"protocol"}, {"log protocol handling"}, DAPLog::Protocol},
+};
+
+static Log::Channel g_log_channel(g_categories,
+ DAPLog::Transport | DAPLog::Protocol);
+
+template <> Log::Channel &lldb_private::LogChannelFor<DAPLog>() {
+ return g_log_channel;
+}
+
+void lldb_dap::InitializeDAPChannel() {
+ Log::Register("lldb-dap", g_log_channel);
+}
diff --git a/lldb/tools/lldb-dap/DAPLog.h b/lldb/tools/lldb-dap/DAPLog.h
new file mode 100644
index 0000000000000..c22cb53655dbf
--- /dev/null
+++ b/lldb/tools/lldb-dap/DAPLog.h
@@ -0,0 +1,33 @@
+//===-- DAPLog.h ------------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_UTILITY_LLDBLOG_H
+#define LLDB_UTILITY_LLDBLOG_H
+
+#include "lldb/Utility/Log.h"
+#include "llvm/ADT/BitmaskEnum.h"
+
+namespace lldb_dap {
+
+enum class DAPLog : lldb_private::Log::MaskType {
+ Transport = lldb_private::Log::ChannelFlag<0>,
+ Protocol = lldb_private::Log::ChannelFlag<1>,
+ LLVM_MARK_AS_BITMASK_ENUM(Protocol),
+};
+
+LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();
+
+void InitializeDAPChannel();
+
+} // end namespace lldb_dap
+
+namespace lldb_private {
+template <> lldb_private::Log::Channel &LogChannelFor<lldb_dap::DAPLog>();
+} // namespace lldb_private
+
+#endif
diff --git a/lldb/tools/lldb-dap/IOStream.cpp b/lldb/tools/lldb-dap/IOStream.cpp
deleted file mode 100644
index c6f1bfaf3b799..0000000000000
--- a/lldb/tools/lldb-dap/IOStream.cpp
+++ /dev/null
@@ -1,65 +0,0 @@
-//===-- IOStream.cpp --------------------------------------------*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "IOStream.h"
-#include "lldb/Utility/IOObject.h"
-#include "lldb/Utility/Status.h"
-#include <fstream>
-#include <string>
-
-using namespace lldb_dap;
-
-bool OutputStream::write_full(llvm::StringRef str) {
- if (!descriptor)
- return false;
-
- size_t num_bytes = str.size();
- auto status = descriptor->Write(str.data(), num_bytes);
- return status.Success();
-}
-
-bool InputStream::read_full(std::ofstream *log, size_t length,
- std::string &text) {
- if (!descriptor)
- return false;
-
- std::string data;
- data.resize(length);
-
- auto status = descriptor->Read(data.data(), length);
- if (status.Fail())
- return false;
-
- text += data;
- return true;
-}
-
-bool InputStream::read_line(std::ofstream *log, std::string &line) {
- line.clear();
- while (true) {
- if (!read_full(log, 1, line))
- return false;
-
- if (llvm::StringRef(line).ends_with("\r\n"))
- break;
- }
- line.erase(line.size() - 2);
- return true;
-}
-
-bool InputStream::read_expected(std::ofstream *log, llvm::StringRef expected) {
- std::string result;
- if (!read_full(log, expected.size(), result))
- return false;
- if (expected != result) {
- if (log)
- *log << "Warning: Expected '" << expected.str() << "', got '" << result
- << "\n";
- }
- return true;
-}
diff --git a/lldb/tools/lldb-dap/IOStream.h b/lldb/tools/lldb-dap/IOStream.h
deleted file mode 100644
index e9fb8e11c92da..0000000000000
--- a/lldb/tools/lldb-dap/IOStream.h
+++ /dev/null
@@ -1,42 +0,0 @@
-//===-- IOStream.h ----------------------------------------------*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef LLDB_TOOLS_LLDB_DAP_IOSTREAM_H
-#define LLDB_TOOLS_LLDB_DAP_IOSTREAM_H
-
-#include "lldb/lldb-forward.h"
-#include "llvm/ADT/StringRef.h"
-#include <fstream>
-#include <string>
-
-namespace lldb_dap {
-
-struct InputStream {
- lldb::IOObjectSP descriptor;
-
- explicit InputStream(lldb::IOObjectSP descriptor)
- : descriptor(std::move(descriptor)) {}
-
- bool read_full(std::ofstream *log, size_t length, std::string &text);
-
- bool read_line(std::ofstream *log, std::string &line);
-
- bool read_expected(std::ofstream *log, llvm::StringRef expected);
-};
-
-struct OutputStream {
- lldb::IOObjectSP descriptor;
-
- explicit OutputStream(lldb::IOObjectSP descriptor)
- : descriptor(std::move(descriptor)) {}
-
- bool write_full(llvm::StringRef str);
-};
-} // namespace lldb_dap
-
-#endif
diff --git a/lldb/tools/lldb-dap/Protocol.cpp b/lldb/tools/lldb-dap/Protocol.cpp
new file mode 100644
index 0000000000000..6f8687ddb5a59
--- /dev/null
+++ b/lldb/tools/lldb-dap/Protocol.cpp
@@ -0,0 +1,191 @@
+//===-- Protocol.cpp --------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "Protocol.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/JSON.h"
+#include <optional>
+#include <utility>
+
+namespace llvm {
+namespace json {
+bool fromJSON(const llvm::json::Value &Params, llvm::json::Value &V,
+ llvm::json::Path P) {
+ V = std::move(Params);
+ return true;
+}
+} // namespace json
+} // namespace llvm
+
+namespace lldb_dap {
+namespace protocol {
+
+enum class MessageType { request, response, event };
+
+bool fromJSON(const llvm::json::Value &Params, MessageType &M,
+ llvm::json::Path P) {
+ auto rawType = Params.getAsString();
+ if (!rawType) {
+ P.report("expected a string");
+ return false;
+ }
+ std::optional<MessageType> type =
+ llvm::StringSwitch<std::optional<MessageType>>(*rawType)
+ .Case("request", MessageType::request)
+ .Case("response", MessageType::response)
+ .Case("event", MessageType::event)
+ .Default(std::nullopt);
+ if (!type) {
+ P.report("unexpected value");
+ return false;
+ }
+ M = *type;
+ return true;
+}
+
+llvm::json::Value toJSON(const Request &R) {
+ llvm::json::Object Result{
+ {"type", "request"},
+ {"seq", R.seq},
+ {"command", R.command},
+ };
+ if (R.rawArguments)
+ Result.insert({"arguments", R.rawArguments});
+ return std::move(Result);
+}
+
+bool fromJSON(llvm::json::Value const &Params, Request &R, llvm::json::Path P) {
+ llvm::json::ObjectMapper O(Params, P);
+ MessageType type;
+ if (!O.map("type", type)) {
+ return false;
+ }
+ if (type != MessageType::request) {
+ P.field("type").report("expected to be 'request'");
+ return false;
+ }
+
+ return O && O.map("command", R.command) && O.map("seq", R.seq) &&
+ O.map("arguments", R.rawArguments);
+}
+
+llvm::json::Value toJSON(const Response &R) {
+ llvm::json::Object Result{{"type", "response"},
+ {"req", 0},
+ {"command", R.command},
+ {"request_seq", R.request_seq},
+ {"success", R.success}};
+
+ if (R.message)
+ Result.insert({"message", R.message});
+ if (R.rawBody)
+ Result.insert({"body", R.rawBody});
+
+ return std::move(Result);
+}
+
+bool fromJSON(llvm::json::Value const &Params, Response &R,
+ llvm::json::Path P) {
+ llvm::json::ObjectMapper O(Params, P);
+ MessageType type;
+ if (!O.map("type", type)) {
+ return false;
+ }
+ if (type != MessageType::response) {
+ P.field("type").report("expected to be 'response'");
+ return false;
+ }
+ return O && O.map("command", R.command) &&
+ O.map("request_seq", R.request_seq) && O.map("success", R.success) &&
+ O.mapOptional("message", R.message) &&
+ O.mapOptional("body", R.rawBody);
+}
+
+llvm::json::Value toJSON(const Event &E) {
+ llvm::json::Object Result{
+ {"type", "event"},
+ {"seq", 0},
+ {"event", E.event},
+ };
+ if (E.rawBody)
+ Result.insert({"body", E.rawBody});
+ if (E.statistics)
+ Result.insert({"statistics", E.statistics});
+ return std::move(Result);
+}
+
+bool fromJSON(llvm::json::Value const &Params, Event &E, llvm::json::Path P) {
+ llvm::json::ObjectMapper O(Params, P);
+ MessageType type;
+ if (!O.map("type", type)) {
+ return false;
+ }
+ if (type != MessageType::event) {
+ P.field("type").report("expected to be 'event'");
+ return false;
+ }
+
+ return O && O.map("event", E.event) && O.mapOptional("body", E.rawBody) &&
+ O.mapOptional("statistics", E.statistics);
+}
+
+bool fromJSON(const llvm::json::Value &Params, ProtocolMessage &PM,
+ llvm::json::Path P) {
+ llvm::json::ObjectMapper O(Params, P);
+ if (!O)
+ return false;
+
+ MessageType type;
+ if (!O.map("type", type))
+ return false;
+
+ switch (type) {
+ case MessageType::request: {
+ Request req;
+ if (!fromJSON(Params, req, P)) {
+ return false;
+ }
+ PM = std::move(req);
+ return true;
+ }
+ case MessageType::response: {
+ Response resp;
+ if (!fromJSON(Params, resp, P)) {
+ return false;
+ }
+ PM = std::move(resp);
+ return true;
+ }
+ case MessageType::event:
+ Event evt;
+ if (!fromJSON(Params, evt, P)) {
+ return false;
+ }
+ PM = std::move(evt);
+ return true;
+ }
+ llvm_unreachable("Unsupported protocol message");
+}
+
+llvm::json::Value toJSON(const ProtocolMessage &PM) {
+ if (auto const *Req = std::get_if<Request>(&PM)) {
+ return toJSON(*Req);
+ }
+ if (auto ...
[truncated]
|
Member
JDevlieghere
left a comment
JDevlieghere
left a comment
There was a problem hiding this comment.
This PR feels like it combines 3 different changes:
- Introducing the Transport class
- Using LLDB's logging
- Introducing the protocol class
Would it be a lot of work to split this up into 3 separate PRs? If not I'm happy to review everything together here. The Transport changes LGTM and the logging changes look pretty straightforward too, though I'm still worried about the increasing amount of lldb_private stuff we're starting to rely on. The largest code change is the introduction of the Protocol class which I'd like to look at in more detail.
| @@ -0,0 +1,110 @@ | |||
| //===-- Transport.cpp -------------------------------------------*- C++ -*-===// | |||
Member
There was a problem hiding this comment.
The *- C++ -* was only meant to go into headers (to tell emacs they were C++ rather than C headers) but there was an RFC to drop it altogether. New files should omit it.
Suggested change
| //===-- Transport.cpp -------------------------------------------*- C++ -*-===// | |
| //===-- Transport.cpp ------------------------------------------------------===// |
Comment on lines
+9
to
+12
| // Debug Adapter Protocol transport layer for encoding and decoding protocol | ||
| // messages. | ||
| // | ||
| //===----------------------------------------------------------------------===// |
Member
There was a problem hiding this comment.
Not sure if this adds much on top of the class doxygen comment.
Comment on lines
+180
to
+184
| // struct ProtocolMessage { | ||
| // MessageType type; | ||
| // int64_t seq; | ||
| // }; | ||
| // enum class MessageType { request, event, response }; |
Member
There was a problem hiding this comment.
I assume this is from before you had std::variant?
| // int64_t seq; | ||
| // }; | ||
| // enum class MessageType { request, event, response }; | ||
| using ProtocolMessage = std::variant<Request, Response, Event>; |
Member
There was a problem hiding this comment.
Given this is in the protocol namespace, how about just Message?
Contributor
Author
Sure, let me split this up. I can start with the protocol classes > logging > transport. |
Contributor
Author
|
Replacing this with #129155 for a smaller set of files to review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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 removes the previous 'OutputStream' and 'InputStream' objects and the new 'Transport' class takes on the responsibility of serializing and encoding messages.
The new Protocol.h file has a dedicated namespace for POD style structures to represent the Debug Adapter Protocol messages.
This should help unblock supporting 'cancel' requests by allowing us to have more detailed type information.