From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57193) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abDCY-0000m7-Vr for qemu-devel@nongnu.org; Wed, 02 Mar 2016 15:16:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abDCV-0004Yb-LR for qemu-devel@nongnu.org; Wed, 02 Mar 2016 15:16:54 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42038) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abDCV-0004Y6-DX for qemu-devel@nongnu.org; Wed, 02 Mar 2016 15:16:51 -0500 References: <1456443528-13901-1-git-send-email-eblake@redhat.com> <1456443528-13901-12-git-send-email-eblake@redhat.com> <87egbsg0qf.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <56D74A31.6000604@redhat.com> Date: Wed, 2 Mar 2016 13:16:49 -0700 MIME-Version: 1.0 In-Reply-To: <87egbsg0qf.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="dAJBUWT8n8Oe2oDLaxnu1delPLCWcUNpS" Subject: Re: [Qemu-devel] [PATCH v2 11/19] qapi: Add type.is_empty() helper 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) --dAJBUWT8n8Oe2oDLaxnu1delPLCWcUNpS Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 03/02/2016 12:04 PM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> And use it in qapi-types and qapi-event. Down the road, we may >> want to lift our artificial restriction of no variants at the >> top level of an event, at which point, inlining our check for >> whether members is empty will no longer be sufficient, but >> adding a check for variants adds verbosity; in the meantime, >> add some asserts in places where we don't handle variants. >=20 > Perhaps I'm just running out of steam for today, but I've read this > twice, and still don't get why adding these assertions goes in the same= > patch as adding the helper, or what it has to do with events. And yet it was the review on the earlier posting that caused me to add asserts; maybe re-reading that thread will help refresh memory, and spur an idea for how to better express it in the commit message: https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04726.html >=20 >> More immediately, the new .is_empty() helper will help fix a bug >> in qapi-visit in the next patch, where the generator did not >> handle an explicit empty type in the same was as a missing type. >=20 > same way [Ever wonder if I intentionally stick in a typo, just to see who will notice? Or maybe it really was a slip of the finger...] >> +++ b/scripts/qapi-event.py >> @@ -39,7 +39,7 @@ def gen_event_send(name, arg_type): >> ''', >> proto=3Dgen_event_send_proto(name, arg_type)) >> >> - if arg_type and arg_type.members: >> + if arg_type and not arg_type.is_empty(): >> ret +=3D mcgen(''' >> QmpOutputVisitor *qov; >> Visitor *v; >=20 > Oh, you don't just add a helper, you actually *change* the condition! > Perhaps the commit message would be easier to understand if it explaine= d > that first. The old condition: arg_type and arg_type.members New condition: arg_type and (arg_type.members or arg_type.variants) But we know there are no variants, since unions cannot (yet) be passed as event 'data', so the condition is the same effect now, and future-proofing for a future patch when I do allow unions in events. >> +++ b/scripts/qapi-types.py >> @@ -90,7 +90,7 @@ struct %(c_name)s { >> # potential issues with attempting to malloc space for zero-lengt= h >> # structs in C, and also incompatibility with C++ (where an empty= >> # struct is size 1). >> - if not (base and base.members) and not members and not variants: >> + if (not base or base.is_empty()) and not members and not variants= : >> ret +=3D mcgen(''' >> char qapi_dummy_for_empty_struct; >> ''') >=20 > I figure the case for the helper based on this patch alone is making th= e > code a bit more future-proof. Suggest you try to explain that in your > commit message, including against what future change exactly you're > proofing the code. And here, bases cannot (yet) have variants, but that's also on my plate of things I'd like to support in the future. >=20 > Haven't reviewed for completeness. >=20 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --dAJBUWT8n8Oe2oDLaxnu1delPLCWcUNpS 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/ iQEcBAEBCAAGBQJW10oxAAoJEKeha0olJ0Nq58IH/j5YZw0qtITDdCo6Y5yfe6c5 rwXodbE40xlJpIqwFOFuSoevSDE16iVW22tmHgSmRk+wBthIpHN8i80eZy1U3Ggh 6WLDlTFrfxXNmYYb35I1q/rHnpdcIRBCcmYUc0EZZe06Hs0mEEBeV3QsQb5YZLiA EYruX2OJTPs1IEXLvxm3B9b2TC4l48OXEpjZZbgen9pNJIrcckPArS9Nf2rxKJX8 owtzBj/OqOARPm6gJthA8aD9SYLcLROzbid9/bcfCXXy1HNAXZAyCWx2a6MiDs1O d/yGzrvmW9tL2EvygtjcKw3rNPKBg2F7nen7UWS6os9qWONOPj7Y8NaXHKVjnvg= =avOf -----END PGP SIGNATURE----- --dAJBUWT8n8Oe2oDLaxnu1delPLCWcUNpS--