[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 03/54] qobject: add literal qobject type
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2 03/54] qobject: add literal qobject type |
Date: |
Tue, 22 Aug 2017 18:24:43 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) |
Marc-André Lureau <address@hidden> writes:
> Hi
>
> ----- Original Message -----
>> Marc-André Lureau <address@hidden> writes:
>>
>> > Promote LiteralQObject from tests/check-qjson.c to qobject/qlit.c,
>> > allowing to statically declare complex qobjects.
>> >
>> > Signed-off-by: Marc-André Lureau <address@hidden>
>>
>> Your patch does more than that! It also
>>
>> * renames the now externally visible identifiers,
>>
>> * adds support for qnull and qnum,
>>
>> * cleans up types (int vs. bool) and style,
>>
>> * makes compare_litqobj_to_qobj() case QTYPE_QNULL and QTYPE_QSTRING
>> more robust, and
>>
>> * fixes bugs in compare_litqobj_to_qobj() case QTYPE_QDICT, QTYPE_QLIST
>> (I think).
>>
>> Squashing the renames into the code motion is tolerable, but the rest
>> isn't, because it makes patch review harder for no benefit at all.
>
> The title said "add" and in the commit message "promote", it's not just a
> "move".
Yes, that's what it says, and it fooled me just fine %-}
> Imho, it's best to take a fresh look at the implementation since it is no
> longer in tests and can be used from qemu/programs.
>
> As such you can review the code as "new" code, and check the corresponding
> tests. Finally, the existing test is simply adapted to that code.
>
>> Moreover, the commit message fails to record the changes. I noticed
>> them only because out of an abundance of caution I checked the patch is
>> just what it's advertised to be: code motion. Well, it isn't.
>>
>> Separate patches, please!
>
> Sure, I can do that, I just thought it was extra overhead given my approach.
Thanks.
- [Qemu-devel] [PATCH v2 18/54] qapi: add 'ifcond' to visitor methods, (continued)
- [Qemu-devel] [PATCH v2 18/54] qapi: add 'ifcond' to visitor methods, Marc-André Lureau, 2017/08/22
- [Qemu-devel] [PATCH v2 39/54] qapi: add #if conditions to generated alternate variants, Marc-André Lureau, 2017/08/22
- [Qemu-devel] [PATCH v2 10/54] block: use qemu_enum_parse() in blkdebug_debug_breakpoint, Marc-André Lureau, 2017/08/22
- [Qemu-devel] [PATCH v2 24/54] qapi-event: add #if conditions to events, Marc-André Lureau, 2017/08/22
- [Qemu-devel] [PATCH v2 14/54] qapi2texi: minor python code simplification, Marc-André Lureau, 2017/08/22
- [Qemu-devel] [PATCH v2 03/54] qobject: add literal qobject type, Marc-André Lureau, 2017/08/22
- [Qemu-devel] [PATCH v2 02/54] qdict: add qdict_put_null() helper, Marc-André Lureau, 2017/08/22
- [Qemu-devel] [PATCH v2 37/54] qapi: 'if' to alternate variant, Marc-André Lureau, 2017/08/22
- [Qemu-devel] [PATCH v2 32/54] qapi: add 'if' to struct members, Marc-André Lureau, 2017/08/22
- [Qemu-devel] [PATCH v2 08/54] hmp: use qapi_enum_parse() in hmp_migrate_set_capability, Marc-André Lureau, 2017/08/22
- [Qemu-devel] [PATCH v2 21/54] qapi-introspect: modify to_qlit() to generate #if code, Marc-André Lureau, 2017/08/22
- [Qemu-devel] [PATCH v2 29/54] qapi: add 'if' to enum members, Marc-André Lureau, 2017/08/22