From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39375) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adKEs-00014H-N5 for qemu-devel@nongnu.org; Tue, 08 Mar 2016 11:12:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adKEn-0002sz-R4 for qemu-devel@nongnu.org; Tue, 08 Mar 2016 11:12:02 -0500 Received: from mx1.redhat.com ([209.132.183.28]:2879) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adKEn-0002st-JN for qemu-devel@nongnu.org; Tue, 08 Mar 2016 11:11:57 -0500 References: <1457194595-16189-1-git-send-email-eblake@redhat.com> <1457194595-16189-6-git-send-email-eblake@redhat.com> <87d1r580q0.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <56DEF9CB.6020601@redhat.com> Date: Tue, 8 Mar 2016 09:11:55 -0700 MIME-Version: 1.0 In-Reply-To: <87d1r580q0.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="CQwRRIJA8p8oDVd0kOCQxMmWmev0dQ5N3" Subject: Re: [Qemu-devel] [PATCH v4 05/10] qapi: Utilize implicit struct visits List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --CQwRRIJA8p8oDVd0kOCQxMmWmev0dQ5N3 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 03/08/2016 08:10 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Rather than generate inline per-member visits, take advantage >> of the 'visit_type_FOO_members()' function for both event and >> command marshalling. This is possible now that implicit >> structs can be visited like any other. >> >> Generated code shrinks accordingly; events initialize a struct >> based on parameters, such as: >> >> >> And command marshalling generates call arguments from a stack-allocate= d >> struct: >=20 > I see qmp-marshal.c shrink from ~5700 lines to ~4300. Neat! Yeah, it nicely balances out the .h getting so much larger, except that the .h gets parsed a lot more by the compiler. >> >> +# Declare and initialize an object 'qapi' using parameters from gen_p= arams() >> +def gen_struct_init(typ): >=20 > It's not just a "struct init", it's a variable declaration with an > initializer. gen_param_var()? >=20 > Name the variable param rather than qapi? Sure, I'm not tied to a specific name. I will point out that we have a potential collision point - any local variable we create here can collide with members of the QAPI struct passed to the event. We haven't hit the collision on any events we've created so far, and it's easy to rename our local variables at such time if we do run into the collision down the road, so I won't worry about it now. >=20 >> + assert not typ.variants >> + ret =3D mcgen(''' >> + %(c_name)s qapi =3D { >> +''', >> + c_name=3Dtyp.c_name()) >> + sep =3D ' ' >> + for memb in typ.members: >> + ret +=3D sep >> + sep =3D ', ' >> + if memb.optional: >> + ret +=3D 'has_' + c_name(memb.name) + sep >> + if memb.type.name =3D=3D 'str': >> + # Cast away const added in gen_params() >> + ret +=3D '(char *)' >=20 > Why is that useful? The compiler complains if you try to initialize a 'char *' member of a QAPI C struct with a 'const char *' parameter. It's no different than the fact that the gen_visit_members() call we are getting rid of also has to cast away const. >=20 >> + ret +=3D c_name(memb.name) >> + ret +=3D mcgen(''' >> + >> + }; >> +''') >> + return ret >=20 > Odd: you use this only in qapi-event.py, not in qapi-commands.py. > Should it live in qapi-event.py then? Yeah, I guess so. I originally put it in qapi.py thinking that I could reuse it in qapi-commands.py, then realized that the two are opposite (event collects parameters into a struct, commands parses a QObject into parameters), so I couldn't share it after all. >> @@ -107,17 +96,24 @@ def gen_marshal_input_visit(arg_type, dealloc=3DF= alse): >> qdv =3D qapi_dealloc_visitor_new(); >> v =3D qapi_dealloc_get_visitor(qdv); >> ''') >> + errp =3D 'NULL' >> else: >> ret +=3D mcgen(''' >> v =3D qmp_input_get_visitor(qiv); >> ''') >> + errp =3D '&err' >> >> - ret +=3D gen_visit_members(arg_type.members, skiperr=3Ddealloc) >=20 > Unless I'm mistaken, this is the only use of skiperr. Follow up with a= > patch to drop the parameter and simplify? Oh, nice. I noticed some cleanups in patch 6/10, but missed this one as a reasonable improvement. In fact, gen_visit_members() is now only used in qapi-visits.py, so maybe I can move it back there (it used to live there before commit 82ca8e469 moved it for sharing with the two files simplified here). --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --CQwRRIJA8p8oDVd0kOCQxMmWmev0dQ5N3 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/ iQEcBAEBCAAGBQJW3vnLAAoJEKeha0olJ0NqmK8H/3s18l2G4OneLkPNL9xO5Out jBUghnbRVvTjGwM1SJFinkbrIHtqPzfoY1Lq53JLPsfClD4EvY9o8g8QhbLMp+KN gBHb9q56PUJeKXG0ZjVPJ/zKLl5Pq1Z7qt1je5odi+c4locXTTsy3szO8XBOvBnq 9n5hiYiW9XmlrbHPG9eSh6TZfYDPrZM0L+yZaFZXSi4eyuw54JudzCMQb5Kq+ceb zx1kB77d2huNHRgSNLo9Oau1AKDri8R+QqLHOV1fApnR5stWZ3zLy9VxTYUBZnhY Encwf+QDgeAYGi9eouxW70VefLja6SeMyX5mKJP/pDW3wMpbQVOABqZzJmagyO8= =jna/ -----END PGP SIGNATURE----- --CQwRRIJA8p8oDVd0kOCQxMmWmev0dQ5N3--