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

[Marc-André Lureau]

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".

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