Hi ----- Original Message ----- > Marc-André Lureau <marcandre.lur...@redhat.com> writes: > > > Promote LiteralQObject from tests/check-qjson.c to qobject/qlit.c, > > allowing to statically declare complex qobjects. > > > > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> > > 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
