[Lldb-commits] [PATCH] D23884: Add StructuredData unit tests; remove JSON parsing string copy

[Zachary Turner via lldb-commits]

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
On Fri, Aug 26, 2016 at 2:24 AM Pavel Labath via lldb-commits <
lldb-commits at lists.llvm.org> wrote:

> labath added a comment.
>
> Thank you for writing the tests. I have two stylistic comments, but
> otherwise looks great.
>
>
> ================
> Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:113
> @@ -105,4 +112,3 @@
>              case 'J':
> -                // 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.
>
> ================
> 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.
>
>
> https://reviews.llvm.org/D23884
>
>
>
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20160826/1528b284/attachment.html>