From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46440) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZgwdN-0002s1-1x for qemu-devel@nongnu.org; Tue, 29 Sep 2015 11:16:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZgwdH-0002gn-Jv for qemu-devel@nongnu.org; Tue, 29 Sep 2015 11:16:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54553) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZgwdH-0002gZ-CR for qemu-devel@nongnu.org; Tue, 29 Sep 2015 11:15:55 -0400 References: <1443497249-15361-1-git-send-email-eblake@redhat.com> <1443497249-15361-16-git-send-email-eblake@redhat.com> <87zj05guv8.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <560AAB29.2030004@redhat.com> Date: Tue, 29 Sep 2015 09:15:53 -0600 MIME-Version: 1.0 In-Reply-To: <87zj05guv8.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Gf7xpuH1bleU0DK2UJAg3kV9EbKnofixp" Subject: Re: [Qemu-devel] [PATCH v6 15/16] qapi: Share gen_err_check() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --Gf7xpuH1bleU0DK2UJAg3kV9EbKnofixp Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/29/2015 08:31 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> qapi-commands had a nice helper gen_err_check(), but did not >=20 > In fact, it still has :) >=20 >> use it everywhere. In fact, using it in more places makes it >> easier to reduce the lines of code used in appending an error >=20 > Suggest "used for generating error checks" >=20 >> check in generated code (previously required a multi-line >> mcgen(), which didn't add any use of parameterization), which >> in turn makes it easier to write the next patch which will >> consolidate another common pattern among the generators. >=20 > I think we should burn some of the whiches ;) Yay - a which-hunt. I'll get the pitchfork. >=20 > Drop the paranthesis? >=20 >> The diffstat of this patch doesn't quite show as big a >> reduction in lines as I had hoped, but that is in part due to >=20 > Almost none... >=20 >> the duplication of some FIXME comments. >> >> Signed-off-by: Eric Blake >=20 > Have you diffed the generated code before and after the patch? Oops, forgot to mention. No change to generated code. >> @@ -170,9 +159,10 @@ static void qmp_marshal_output_%(c_name)s(%(c_typ= e)s ret_in, QObject **ret_out, >> >> v =3D qmp_output_get_visitor(qov); >> visit_type_%(c_name)s(v, &ret_in, "unused", &err); >> - if (err) { >> - goto out; >> - } >> +''', >> + c_type=3Dret_type.c_type(), c_name=3Dret_type.c_name(= )) >> + ret +=3D gen_err_check() >> + ret +=3D mcgen(''' >> *ret_out =3D qmp_output_get_qobject(qov); >> >> out: >=20 > Here's a case that becomes more verbose, so it's not just comment > duplication. Also becomes a bit harder to read, I think. Due to splitting a larger chunk into smaller pieces to inject the error in the middle. I guess we could pick and choose to use the function only where it doesn't split an already-existing large block, in order to maximize the diffstat ratio rather than in favor of eliminating the common code pattern duplication? >=20 > I don't know. Could be worthwhile if it really makes further work > easier. >=20 > To really cut the verbosity, I figure we'd have to do something more > radical, like having cgen() recognize a (short!) pattern and replace it= > with a full-blown error check. Not sure that's actually a good idea, > though :) In v5, it was only used by the shared gen_visit_fields(), until I added uses of it later in the series to make anonymous base classes of a flat union easier to implement. I guess the conservative thing is to scale this patch back a bit (move the function to the common location, and use it where it makes an obvious difference to the diffstat, but leaving other places alone for now). Should I try that for v7? --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --Gf7xpuH1bleU0DK2UJAg3kV9EbKnofixp 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/ iQEcBAEBCAAGBQJWCqspAAoJEKeha0olJ0NqB/AH/3oUVsRpwi5zi+ofBSmVfYjK j3B875k0D0GMm4AoZd5J/85GbtYlhFsDU5CirFqcUIx7U7Kxjv1NusVlsTMWTvC6 ZxuKxQGXXKBEmvYrCUSLcWxB3XFxjGskWr8zYe1sitkVb+6UNSDXZUCmV4YMNfs0 HiqMDB+tHFbwL/3utIm4Cgrpvv2E+qVb+41DJmng/VEELnkOR9L2s0F36hhsLeKY FUXjTIrW98g4SmhZbnTbjcyZKNBUZUlVbia7NWmBhcvENiQMN8vZaMcZDEH05Rif 7EoFYFERG23vlFvFvzGyoaFRFDqlIBUqi2KKzL8lKFCJgzqGut/QyTmEtrmg0Bo= =DI4g -----END PGP SIGNATURE----- --Gf7xpuH1bleU0DK2UJAg3kV9EbKnofixp--