From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43036) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZIM9R-0006Rm-Rc for qemu-devel@nongnu.org; Thu, 23 Jul 2015 15:27:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZIM9N-000484-RK for qemu-devel@nongnu.org; Thu, 23 Jul 2015 15:27:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50238) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZIM9N-00047P-Jw for qemu-devel@nongnu.org; Thu, 23 Jul 2015 15:27:25 -0400 References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-38-git-send-email-armbru@redhat.com> From: Eric Blake Message-ID: <55B14005.9090909@redhat.com> Date: Thu, 23 Jul 2015 13:27:01 -0600 MIME-Version: 1.0 In-Reply-To: <1435782155-31412-38-git-send-email-armbru@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="5TlqQtrxQurDX36xfmf7pu7nu82goGU12" Subject: Re: [Qemu-devel] [PATCH RFC v2 37/47] qapi: De-duplicate parameter list generation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: kwolf@redhat.com, berto@igalia.com, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --5TlqQtrxQurDX36xfmf7pu7nu82goGU12 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 07/01/2015 02:22 PM, Markus Armbruster wrote: > Generated qapi-event.[ch] lose line breaks. No change otherwise. For example, -void qapi_event_send_block_image_corrupted(const char *device, - bool has_node_name, - const char *node_name, - const char *msg, - bool has_offset, - int64_t offset, - bool has_size, - int64_t size, - bool fatal, - Error **errp) +void qapi_event_send_block_image_corrupted(const char *device, bool has_node_name, const char *node_name, const char *msg, bool has_offset, int64_t offset, bool has_size, int64_t size, bool fatal, Error **errp) You know, I'd find it a bit more appealing if you had merged the duplicate code in the _other_ direction. That is, qapi-event's wrapped lines (usually) fit in 80 columns, and it would be nice if qapi-visit's did the same. Yeah, avoiding line wraps consumes fewer source bytes (fewer runs of spaces), but the space isn't being wasted by storing generated files in git, nor does the C compiler care which layout we use. And honestly, it's easier to spot changes in a vertical list than it is on a long horizontal line, if a parameter gets added (or removed, although adding is the more likely action with qapi). >=20 > Signed-off-by: Markus Armbruster > --- > scripts/qapi-commands.py | 11 ++--------- > scripts/qapi-event.py | 18 +++--------------- > scripts/qapi.py | 16 ++++++++++++++++ > 3 files changed, 21 insertions(+), 24 deletions(-) I'm a fan of de-duplication, so I'll review this on its merits; but I'm omitting R-b on this round in hopes that you buy my argument to merge in the other direction (make qapi-event's implementation the common one). >=20 > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > index d57f8d4..2dae425 100644 > --- a/scripts/qapi-commands.py > +++ b/scripts/qapi-commands.py > - args=3Dargstr) > + params=3Dgen_params(args, 'Error **errp')) Caller 1. > +++ b/scripts/qapi-event.py > + return 'void qapi_event_send_%(c_name)s(%(param)s)' % { > + 'c_name': c_name(name.lower()), > + 'param': gen_params(data, 'Error **errp')} Caller 2. > =20 > def gen_event_send_decl(name, data): > return mcgen(''' > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 4d47214..c6a5ddc 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -1384,6 +1384,22 @@ extern const char *const %(c_name)s_lookup[]; > c_name=3Dc_name(name)) > return ret > =20 > +def gen_params(args, extra): > + if not args: > + return extra Both callers pass the same 'extra' - do you need it to be parameterized, or can it just be generated as a constant here? (I guess it depends on what happens with the later introspection patch, which may become caller = 3). > + assert not args.variants This assert will trip if you don't fix events to reject 'data':'Union' :)= > + ret =3D "" > + sep =3D "" > + for memb in args.members: > + ret +=3D sep > + sep =3D ", " > + if memb.optional: > + ret +=3D "bool has_%s, " % c_name(memb.name) Didn't you just provide a patch that used '' rather than "" for all generated C constructs? This violates that paradigm. > + ret +=3D "%s %s" % (memb.type.c_type(is_param=3DTrue), c_name(= memb.name)) > + if extra: > + ret +=3D sep + extra > + return ret > + To produce line breaks, you could have to add a parameter so that callers can pass in the starting column for each wrapped argument, and then you'd have sep =3D ',\n' + ''.ljust(len). Or even have the caller choose its own separator (", " vs. ",\n "), if you don't want to have a diff in the generated output (but I think consistent generated output is nicer). --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --5TlqQtrxQurDX36xfmf7pu7nu82goGU12 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJVsUAFAAoJEKeha0olJ0NqDXkH/3DEL3HxZe3GXQ/X2csX3qJt 4G6+uC9gv9hMPGIyCJD9BWImZ9RHpXY12fePg0Q6/IhBXOd5n9bolPbkUtBGurYF kitS9pWqHKW4RALyJwsltBsnoLVYDDhzvPObtZqaFglEsJNqbsdNk1QAfj51BcJP jv12zXrwyBnawzdCLI49HyKjHoKEAUTTYpM3YB+ExvgdsUJcPz7R6G56tcwoO4Cm Cc6L552+JZEQrY/EpT/rL0nNmS0Ebvnt8xR9SbuS9k9hy4mCK0QdGkQ7kJmYjBKY DyYGN1xKap/kao/ixUTPqisZWaPy5RN/QybQl7nmLNngoaZofEI/lVRyBU/B5xU= =avNO -----END PGP SIGNATURE----- --5TlqQtrxQurDX36xfmf7pu7nu82goGU12--