labath added inline comments.
================
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3203
+ error.SetError(response.GetError(), eErrorTypeGeneric);
+ LLDB_LOG(log, "Target does not support Tracing , error {0}",
error.AsCString());
} else {
----------------
AsCString unnecessary
================
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3332
} else {
- error.SetError(response.GetError(), eErrorTypeGeneric);
+ if (GetErrorStringInPacketSupported())
+ error = response.GetStatus();
----------------
I don't think every packet function should need to write these four lines of
code. This should be abstracted away. Ideally I'd like to see this as a method
on the response class (i.e., simply `error = response.GetStatus()`), but if
it's hard to plumb that information into the class cleanly, I can imagine it
being a method on the communication class as well (`error =
this->GetStatus(response)`)
update: It looks like the GetStatus function already supports that: Why not
just write this as `error = response.GetStatus()` ???
================
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:104
+GDBRemoteCommunicationServer::SendErrorResponse(Status &error) {
+ char errc[16];
+ int errc_len = ::snprintf(errc, sizeof(errc), "E%2.2x", error.GetError());
----------------
Condensed version without dodgy C functions:
`return SendPacketNoLock(llvm::formatv("E{0:x-2};{1}", error.GetError(),
error).str());`
================
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:110
+ packet += ";";
+ packet += std::string(error.AsCString());
+ return SendPacketNoLock(packet);
----------------
Aren't you supposed to send these only if the client enabled the error response?
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h:64
+ PacketResult SendErrorResponse(Status &error);
+
----------------
Why the reference?
================
Comment at: source/Utility/StringExtractorGDBRemote.cpp:463
+
+ std::string error_messg ("Error 47");
+ size_t str_index = m_packet.find(';', m_index);
----------------
replace 47 with the actual error code
https://reviews.llvm.org/D34945
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
