Marc-André Lureau <marcandre.lur...@redhat.com> writes: > 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".
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.
