From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38958) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zw8py-0001BM-Q5 for qemu-devel@nongnu.org; Tue, 10 Nov 2015 08:19:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zw8ps-0005gC-QA for qemu-devel@nongnu.org; Tue, 10 Nov 2015 08:19:50 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60369) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zw8ps-0005fq-IU for qemu-devel@nongnu.org; Tue, 10 Nov 2015 08:19:44 -0500 References: <1446791754-23823-1-git-send-email-eblake@redhat.com> <1446791754-23823-25-git-send-email-eblake@redhat.com> <87r3jzdy8p.fsf@blackfin.pond.sub.org> <56418158.9000403@redhat.com> <87k2pq2p1d.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <5641EEEA.2010903@redhat.com> Date: Tue, 10 Nov 2015 06:19:38 -0700 MIME-Version: 1.0 In-Reply-To: <87k2pq2p1d.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="5NRKHfPCX4K7C4AoaV3rjxEO3QhXRwBpp" Subject: Re: [Qemu-devel] [PATCH v10 24/30] qapi: Factor out QAPISchemaObjectType.check_clash() 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) --5NRKHfPCX4K7C4AoaV3rjxEO3QhXRwBpp Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/10/2015 02:15 AM, Markus Armbruster wrote: >> On the other hand, we've been arguing that check() should populate >> everything after construction prior to anything else being run; and no= t >> running Variant.type.check() during Variants.check() of flat unions >> feels like we may have a hole (a flat union will have to inline its >> types to the overall JSON object, and inlining types requires access t= o >> type.members - but as written, we aren't populating them until >> Variants.check_clash()). I can play with hoisting the type.check() ou= t >> of type.check_clash() and instead keep base.check() in type.check(), a= nd >> add variant.type.check() in Variants.check() (but only for unions, not= >> for alternates), if you are interested. >=20 > My "qapi: Factor out QAPISchemaObjectTypeMember.check_clash()" adds > QAPISchemaObjectTypeMember.check_clash() without changing the common > protocol. The new QAPISchemaObjectTypeMember.check_clash() is merely a= > helper for QAPISchemaObjectType.check(). >=20 > The two .check_clash() you add (one in this patch, one in the previous > one) are different: both contain calls of QAPISchemaObjectType.check().= >=20 > I feel the .check() calls are too important to be buried deep like that= =2E > I'd stick to prior practice and put the .check() calls right into > .check(). Obviously, the .check_clash() methods may only called after > .check() then, but that's nothing new. >=20 > Fixup for your previous patch: >=20 > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 4c56935..357127d 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -1065,7 +1065,6 @@ class QAPISchemaObjectTypeVariants(object): > vseen =3D dict(seen) > assert isinstance(v.type, QAPISchemaObjectType) > assert not v.type.variants # not implemented > - v.type.check(schema) > for m in v.type.members: > m.check_clash(vseen) > =20 > @@ -1077,6 +1076,7 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjec= tTypeMember): > def check(self, schema, tag_type): > QAPISchemaObjectTypeMember.check(self, schema) > assert self.name in tag_type.values > + self.type.check(schema) > =20 Won't quite work. You are right that we must call self.type.check(schema) for variants used by a union; but calling it for ALL variants used by an alternate is wrong, because self.type for at least one branch of an alternate will not be an instance of QAPISchemaObjectType. However, I'm currently testing whether it is safe to check to just blindly check an object branch of an alternate, if present (and that should not lead to cycles, since alternates have no base class and since we don't allow one alternate type as a variant of another alternate), in which case the fixup for 23/30 is more like: diff --git i/scripts/qapi.py w/scripts/qapi.py index a005c87..25fa642 100644 --- i/scripts/qapi.py +++ w/scripts/qapi.py @@ -1065,7 +1065,6 @@ class QAPISchemaObjectTypeVariants(object): vseen =3D dict(seen) assert isinstance(v.type, QAPISchemaObjectType) assert not v.type.variants # not implemented - v.type.check(schema) for m in v.type.members: m.check_clash(vseen) @@ -1077,6 +1076,8 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): def check(self, schema, tag_type): QAPISchemaObjectTypeMember.check(self, schema) assert self.name in tag_type.values + if isinstance(self.type, QAPISchemaObjectType): + self.type.check(schema) # This function exists to support ugly simple union special cases # TODO get rid of them, and drop the function @@ -1098,6 +1099,8 @@ class QAPISchemaAlternateType(QAPISchemaType): def check(self, schema): self.variants.tag_member.check(schema) + # Not calling self.variants.check_clash(), because there's + # nothing to clash with self.variants.check(schema, {}) def json_type(self): --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --5NRKHfPCX4K7C4AoaV3rjxEO3QhXRwBpp 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/ iQEcBAEBCAAGBQJWQe7qAAoJEKeha0olJ0NqYIkH/26wTiSHH+20ai7QuAghCZHi UwXGDFd6kpihvdisI2wy0qwSqJqhSGycUBnV44KsWCT1IEu5+clHJMfbqmWKFHQA udoAYtb5FqmlsvnpXjoT3QvH3FpdKqGEebmKkvlNqbMK1KqBveOQ6m40Kkl0IV6r LWiv2rlu4CyURdCBZXTydKnENNma67TbEjm5A1nqjuyuR3lI8+rSaKgUHe2c4+/s 4mz+wk/+dmky5L3nmqTaxT/cQSimgiHJ1nLqudCf2cRQ72yqzxRAm9b/hOV9DX6t 0raek7jLGjOdk9+Z4bO+yj76rZjbL4gbWLqDHrdJ9NEurXlovcbAf9OXf6Xc3J8= =0yLi -----END PGP SIGNATURE----- --5NRKHfPCX4K7C4AoaV3rjxEO3QhXRwBpp--