Re: [Qemu-devel] [PATCH 2/4] check-qjson: Fix up a few bogus comments

[Laszlo Ersek <lersek@redhat.com>]

I don't understand what's going on here.

On 03/14/13 18:49, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/check-qjson.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
> index ec85a0c..852124a 100644
> --- a/tests/check-qjson.c
> +++ b/tests/check-qjson.c
> @@ -4,7 +4,7 @@
>   *
>   * Authors:
>   *  Anthony Liguori   <aliguori@us.ibm.com>
> - *  Markus Armbruster <armbru@redhat.com>,
> + *  Markus Armbruster <armbru@redhat.com>
>   *
>   * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
>   * See the COPYING.LIB file in the top-level directory.
> @@ -462,8 +462,7 @@ static void utf8_string(void)
>          },
>          /* 3.3.4  5-byte sequence with last byte missing (U+0000) */
>          {
> -            /* invalid */
> -            "\"\xF8\x80\x80\x80\"", /* bug: not corrected */
> +            "\"\xF8\x80\x80\x80\"",

In this test case, we use an invalid UTF-8 sequence in a JSON string
literal (json_in). So "/* invalid */" could be justified; perhaps it's
just too laconic.

The "/* bug: not corrected */" comment seems indeed wrong, "json_in" is
*input*, there's nothing to correct on it.

>              NULL,                   /* bug: rejected */

When the JSON parser rejects the invalid sequence, it's actually
correct. So why the "bug" comment? Are we expecting (according to the
comment in utf8_string()) U+FFFD?


>              "\"\\u8000\\uFFFF\"",   /* bug: want "\"\\uFFFF\"" */
>              "\xF8\x80\x80\x80",

In this test we're trying to format a UTF-8 byte sequence (utf8_in) as a
JSON string. The source is invalid. The JSON formatter should either
reject it, or emit an U+FFFD in its place. The actual JSON output is
probably wrong, hence the "bug" part is OK, but I thought what we
expected is not U+FFFF but U+FFFD. Hence that part of the comment is
wrong. ... Hm, OK the leading comment has some notes on this as well.

So what your patch does here is:
- remove the halfway OK comment "/* invalid */" -- I think it wasn't
really wrong, but I won't miss it,
- removes an in fact bogus comment,
- (removes a runaway comma).

My eyes are bleeding.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>