Re: [Qemu-devel] [PATCH v2 03/54] qobject: add literal qobject type

[Markus Armbruster]

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.