From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40541) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZgaXc-0005XG-DW for qemu-devel@nongnu.org; Mon, 28 Sep 2015 11:40:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZgaXX-0001CY-Dh for qemu-devel@nongnu.org; Mon, 28 Sep 2015 11:40:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37617) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZgaXW-0001CO-UK for qemu-devel@nongnu.org; Mon, 28 Sep 2015 11:40:31 -0400 References: <1442872682-6523-1-git-send-email-eblake@redhat.com> <1442872682-6523-11-git-send-email-eblake@redhat.com> <87fv1zoync.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <56095F68.60409@redhat.com> Date: Mon, 28 Sep 2015 09:40:24 -0600 MIME-Version: 1.0 In-Reply-To: <87fv1zoync.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="SItSQnUOeTdm7LOc0rL29lDg4j7QpQDmt" Subject: Re: [Qemu-devel] [PATCH v5 10/46] qapi: Merge generation of per-member visits List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: marcandre.lureau@redhat.com, DirtY.iCE.hu@gmail.com, qemu-devel@nongnu.org, ehabkost@redhat.com, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --SItSQnUOeTdm7LOc0rL29lDg4j7QpQDmt Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/28/2015 12:17 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Consolidate the code between visit, command marshalling, and >> event generation that iterates over the members of a struct. >> It reduces code duplication in the generator, with no change to >> generated marshal code, slightly more verbose visit code: >> >> | visit_optional(v, &(*obj)->has_device, "device", &err); >> |- if (!err && (*obj)->has_device) { >> |- visit_type_str(v, &(*obj)->device, "device", &err); >> |- } >> | if (err) { >> | goto out; >> | } >> |+ if ((*obj)->has_device) { >> |+ visit_type_str(v, &(*obj)->device, "device", &err); >> |+ if (err) { >> |+ goto out; >> |+ } >> |+ } >=20 > I think the more verbose code is easier to understand, because it check= s > for errors exactly the same way as we do all the time, mimimizing > cognitive load. And then I shorten it later in 27/46, but the shorter form is consistent to all three due to this refactor into a common helper. >=20 >> and slightly more verbose event code (recall that the qmp >> output visitor has a no-op visit_optional()): >> >> |+ visit_optional(v, &has_offset, "offset", &err); >> |+ if (err) { >> |+ goto out; >> |+ } >=20 > If we had a written contract, I suspect not calling visit_optional() > would be a bug. Indeed - we got lucky that the output visitor's visit_optional() was a no-op. I'll make that fact more obvious in the commit message. >> >> -def gen_err_check(err): >> - if not err: >> - return '' >> - return mcgen(''' >> -if (%(err)s) { >> - goto out; >> -} >> -''', >> - err=3Derr) >> - >> - >=20 > Only code motion. I'm actually debating about splitting the move of this helper function into its own patch, and using it in more places. Part of my debate is that I'd rather go with: def gen_err_check(err=3D'err', label=3D'out'): if not err: return '' return mcgen(''' if (%(err)s) { goto %(label)s; } ''', err=3Derr, label=3Dlabel) so that it is applicable in more places, and so that callers don't have to worry about push_indent()/pop_indent() if it is at the default usage of 4 spaces (right now, all callers have to push, and not just callers at 8 spaces where it is embedded inside an 'if' block). Hmm, and just writing that, I'm wondering if we should fix mcgen() to eat leading whitespace on any final blank line [as a separate patch], since at least for me, emacs wants me to indent as: return mcgen(''' code ''', args) rather than with the closing ''' flush left. >> -''') >> + ret +=3D gen_visit_fields(arg_type.members, '', False, errarg) >=20 > Perhaps a bit neater: make parameters prefix=3D'', need_cast=3DFalse, a= nd > say prefix=3D... and need_cast=3DTrue in the one call where you need it= =2E Good idea, will work it into my v6. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --SItSQnUOeTdm7LOc0rL29lDg4j7QpQDmt 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/ iQEcBAEBCAAGBQJWCV9oAAoJEKeha0olJ0NqUlIH/RezQzsrmcnbAn7nwnqwcQVT gbwZUzyKY+dzXbQl3wsfbtO+DV7Uy/95oCQWHidMWOsv0g8nlzOhqIDnchUUv2nK 5p0cDiE5hg76Rwuv10zfUNf6FBu84yUYTdA56R92h2YvA7eNz+yFQbWWDoCwbTEe Qn4VeTQffmlojvaa3SZOZ0JFpMT+UvZSw05g3BbU1JZmav3ypYDN54y2rf1MMU9s /WCvpLhwk9cdlyKHUUxEOOBOo/2Zx6mjFEthoh/yh8lD7l//QojGtPOxhLphP44U To8rCAi4DROIlQf6qWW1dPTA9/lMRXvlAHziRp1YauLyXTgqOqhR4LQZq4s3+6k= =pCAO -----END PGP SIGNATURE----- --SItSQnUOeTdm7LOc0rL29lDg4j7QpQDmt--