[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I am generally happy with this, just a couple of things I noticed below:



================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3329
     } else {
-      error.SetError(response.GetError(), eErrorTypeGeneric);
+        error = response.GetStatus();
     }
----------------
The indenting look wrong. Please run the patch through clang-format before submission.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h:158
 
+  PacketResult Handle_QErrorStringInPacketSupported(StringExtractorGDBRemote &packet);
+
----------------
It doesn't look like you're implementing this anywhere. Please remove.


================
Comment at: source/Utility/StringExtractorGDBRemote.cpp:26
+      if (m_packet[3] == ';') {
+        auto err_string = m_packet.substr(4);
+        for(auto e : err_string)
----------------
Please use StringRef here. No need to copy the string just to examine it's contents.


================
Comment at: source/Utility/StringExtractorGDBRemote.cpp:29
+          if (!isxdigit(e))
+            break;
+        return eError;
----------------
This will break out of the inner for loop, which is probably not what you intended.


https://reviews.llvm.org/D34945