Unless we have a good reason to use const char* such as interoperability with a C api I would strongly prefer we standardize on llvm::StringRef<br><div class="gmail_quote"><div dir="ltr">On Fri, Aug 26, 2016 at 2:24 AM Pavel Labath via lldb-commits <<a href="mailto:lldb-commits@lists.llvm.org">lldb-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">labath added a comment.<br>
<br>
Thank you for writing the tests. I have two stylistic comments, but otherwise looks great.<br>
<br>
<br>
================<br>
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:113<br>
@@ -105,4 +112,3 @@<br>
             case 'J':<br>
-                // Asynchronous JSON packet, destined for a<br>
-                // StructuredDataPlugin.<br>
             {<br>
+                // Verify this J packet is a JSON-async: packet.<br>
----------------<br>
I'd like to move this code to a separate function. The main job of `SendContinuePacketAndWaitForResponse` is dealing with the thread synchronization issues, which is tricky enough without having json parsing in the middle of it.<br>
<br>
================<br>
Comment at: unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp:371<br>
@@ +370,3 @@<br>
+    ASSERT_EQ("T01", response.GetStringRef());<br>
+    ASSERT_EQ(fix.delegate.structured_data_entries.size(), 1);<br>
+<br>
----------------<br>
Please put the "expected" values first (`ASSERT_NE(expected, actual)`). Otherwise the error message will come out wrong when the comparison fails. The same issue is present in a number of other checks.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D23884" rel="noreferrer" target="_blank">https://reviews.llvm.org/D23884</a><br>
<br>
<br>
<br>
_______________________________________________<br>
lldb-commits mailing list<br>
<a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits</a><br>
</blockquote></div>