[Lldb-commits] [PATCH] D23884: Add StructuredData unit tests; remove JSON parsing string copy
Todd Fiala via lldb-commits
lldb-commits at lists.llvm.org
Fri Aug 26 08:50:48 PDT 2016
tfiala added inline comments.
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:113
@@ -105,4 +112,3 @@
- // Asynchronous JSON packet, destined for a
- // StructuredDataPlugin.
+ // Verify this J packet is a JSON-async: packet.
> 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.
Sure, sounds like a good change.
Comment at: unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp:371
@@ +370,3 @@
+ ASSERT_EQ("T01", response.GetStringRef());
+ ASSERT_EQ(fix.delegate.structured_data_entries.size(), 1);
> 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.
Okay. That's somewhat unfortunate that Python unittest's messages are geared for the reverse:
I will reverse these, thanks for catching that!
More information about the lldb-commits