On 04/17/2015 10:01 AM, Max Reitz wrote: > On 27.03.2015 20:20, John Snow wrote: >> In general, since transactions may reference QMP function helpers, >> it would be nice for them to sit beneath them. >> >> This will avoid the need for forward declaring any QMP interfaces, >> which would be aggravating to update in so many places. >> >> Signed-off-by: John Snow <js...@redhat.com> >> --- >> blockdev.c | 2581 >> ++++++++++++++++++++++++++++++------------------------------ >> 1 file changed, 1292 insertions(+), 1289 deletions(-) > > I'm not so sure about this patch. I mean... 2581 changes? :-/ > > If you (or someone else) can convince me that forward declarations are > really worse than this (is it more aggravating than having a patch with > this diffcount?), well...
I like avoiding forward declarations of static functions, where it makes sense, but I'm not going to insist on reordering code if it makes things ugly. > > But even then, I'd strongly vote for removing dropping all functional > changes beside the code movement from this patch. Because I'm seeing this: > > 2096c2096,2097 > < error_set(errp, QERR_INVALID_PARAMETER_VALUE, "granularity", > "power of 2"); > --- >> error_set(errp, QERR_INVALID_PARAMETER_VALUE, >> "granularity", "power of 2"); Indeed - you WANT code motion patches to be as easy as possible to review. From http://wiki.qemu.org/Contribute/SubmitAPatch "Ideally, a code motion patch can be reviewed by doing git format-patch --stdout -1 > patch; diff -u <(sed -n 's/^-//p' patch) <(sed -n 's/^\+//p' patch), to focus on the few changes that weren't wholesale code motion." > Probably sensible changes, but if you really, really, really want to go > for this code movement, please apply them in an own patch before this > one so that this one really is nothing but code movement. > > Max > > -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
