From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38653) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZsCcA-0007l3-Pf for qemu-devel@nongnu.org; Fri, 30 Oct 2015 12:33:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZsCc7-000467-IK for qemu-devel@nongnu.org; Fri, 30 Oct 2015 12:33:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45232) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZsCc7-00045r-BO for qemu-devel@nongnu.org; Fri, 30 Oct 2015 12:33:15 -0400 References: <1446052473-19170-1-git-send-email-eblake@redhat.com> <1446052473-19170-6-git-send-email-eblake@redhat.com> <87lhakjz3d.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <56339BB1.1090800@redhat.com> Date: Fri, 30 Oct 2015 10:32:49 -0600 MIME-Version: 1.0 In-Reply-To: <87lhakjz3d.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="HdA0fcAxp1hfnuDbocVXVrhvxtLtGaa82" Subject: Re: [Qemu-devel] [PATCH v8 05/17] qapi: Track simple union tag in object.local_members 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) --HdA0fcAxp1hfnuDbocVXVrhvxtLtGaa82 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/30/2015 06:54 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> We were previously creating all unions with an empty list for >> local_members. However, it will make it easier to unify struct >> and union generation if we include the generated tag member in >> local_members. That way, we can have a common code pattern: >> visit the base (if any), visit the local members (if any), visit >> the variants (if any). The local_members of a flat union >> remains empty (because the discriminator is already visited as >> part of the base). >=20 > Keeping the implicit tag implicit by not including it in local_members > was a conscious design decision, but if including it makes unifying > struct and union into objects easier, go right ahead. >=20 >> The various front end entities then map as follows: >> struct: optional base, optional local_members, no variants >> simple union: no base, one-element local_members, variants with tag_me= mber >> from local_members >> flat union: base, no local_members, variants with tag_member from base= >> alternate: no base, no local_members, variants >> >> With the new local members, we require a bit of finesse to >> avoid assertions in the clients. No change to generated code. >> >> Signed-off-by: Eric Blake >> >> +++ b/scripts/qapi-types.py >> @@ -269,7 +269,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):= >> def visit_object_type(self, name, info, base, members, variants):= >> self._fwdecl +=3D gen_fwd_object_or_array(name) >> if variants: >> - assert not members # not implemented >> + if members: >> + assert len(members) =3D=3D 1 >> + assert members[0] =3D=3D variants.tag_member >> self.decl +=3D gen_union(name, base, variants) >> else: >> self.decl +=3D gen_struct(name, base, members) >=20 > The assertion checks that not passing members to gen_union() won't lose= > any. Before the patch, we assert there are none. After the patch, we > assert there's either none or variants.tag_member. Before ans after, > gen_union() takes care of variants.tag_member. Okay. >=20 > Let's keep the comment, though. Perhaps like this: >=20 > # Members other than variants.tag_member not implemente= d > assert len(members) =3D=3D 1 > assert members[0] =3D=3D variants.tag_member Indeed, that sounds nicer. >=20 > I hope this the whole conditional will eventually be replaced by >=20 > self.decl +=3D gen_object(name, base, members, variants) Yes. It goes away in 6/17 for qapi-types (as you already saw), and I have a later patch queued that likewise makes it go away for qapi-visit (but not in subset C or D, since there's still a bit more to clean up there first). >> def check(self, schema, members, seen): >> - if self.tag_name: >> + if self.tag_name: # flat union >> self.tag_member =3D seen[self.tag_name] >> - else: >> + elif seen: # simple union >> + assert self.tag_member in seen.itervalues() >> + else: # alternate >> self.tag_member.check(schema, members, seen) >> assert isinstance(self.tag_member.type, QAPISchemaEnumType) >> for v in self.variants: >=20 > The test for simple union is hackish. >=20 > I guess you want to bypass self.tag_member.check() when for simple > unions, because it's now checked when QAPISchemaObjectType.check() call= s > QAPISchemaObjectTypeMember.check() for each of self.local_members. >=20 > Could we instead make QAPISchemaAlternateType.check() call > QAPISchemaObjectTypeMember.check() for its implicit tag, and drop the > else here? I debated about that when writing my patch. Yes, I can make that change (it seems a little bit unclean to be calling self.variants.tag_member.check() in QAPISchemaAlternateType, but not too much worse; and adding a comment might help). --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --HdA0fcAxp1hfnuDbocVXVrhvxtLtGaa82 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/ iQEcBAEBCAAGBQJWM5uxAAoJEKeha0olJ0Nq7PQH/2mIJsV3CmnGKIiwUwFAviJH JAz2m7aEUJu7SvyhF9IK7xTj52zKw0mgkJDK2aCe2ZoIF3ZkymXAJg+7FetZg1s9 pb8eeDMgg0Qp0ppExPN2H0C/0O2jWNhascFjqcXZaaeBzDZWdJTKX+A7Obx5FGms 84M2tsbFtegISuxIdikWCAN1b2RlSzHHVi3BRNsOUhXqPzO3NELi8YhVs+LI0PJq 42gN9Asindzyz9dkRA89sIume6mP1qKLM+9apEBtI2dxd/pNszvhTwnAPad2K8vY LmwFId+IBFvuWkcrfVS4Tz+E3kUldkvE3SGqs+9h5ySbth9zfL9p4GlfGXnyY+E= =qnbe -----END PGP SIGNATURE----- --HdA0fcAxp1hfnuDbocVXVrhvxtLtGaa82--