From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39810) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwJMq-0001aP-Do for qemu-devel@nongnu.org; Tue, 10 Nov 2015 19:34:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZwJMn-0005su-5X for qemu-devel@nongnu.org; Tue, 10 Nov 2015 19:34:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:50780) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwJMm-0005sn-Tj for qemu-devel@nongnu.org; Tue, 10 Nov 2015 19:34:25 -0500 References: <1446791754-23823-1-git-send-email-eblake@redhat.com> <1446791754-23823-29-git-send-email-eblake@redhat.com> <87611bdwxq.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <56428D09.4010908@redhat.com> Date: Tue, 10 Nov 2015 17:34:17 -0700 MIME-Version: 1.0 In-Reply-To: <87611bdwxq.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Bvs2QkgGRBNGNpM3miKHgFRgOvVO0fKP4" Subject: Re: [Qemu-devel] [PATCH v10 28/30] qapi: Detect collisions in C member names 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) --Bvs2QkgGRBNGNpM3miKHgFRgOvVO0fKP4 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/09/2015 08:17 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Detect attempts to declare two object members that would result >> in the same C member name, by keying the 'seen' dictionary off >> of the C name rather than the qapi name. It also requires passing >> info through the check_clash() methods. >> >> This addresses a TODO and fixes the previously-broken >> args-name-clash test. The resulting error message demonstrates >> the utility of the .describe() method added previously. No change >> to generated code. >> >> Signed-off-by: Eric Blake >> >> --- >> +++ b/scripts/qapi.py >> @@ -975,21 +975,24 @@ class QAPISchemaObjectType(QAPISchemaType): >> seen =3D OrderedDict() >> if self._base_name: >> self.base =3D schema.lookup_type(self._base_name) >> - self.base.check_clash(schema, seen) >> + self.base.check_clash(schema, self.info, seen) >=20 > Note that if this one reports an error for self.info, something went > wrong. We first run self.base.check(), which we survive only when > self.base doesn't have clashing members. Would be easier to see if > self.base.check() wasn't hiding in self.base.check_clash(). Hmm. I could pass None as a way of asserting that it won't fail. But I think that's a bit more confusing, so I'll probably just leave it alone. >> - def check_clash(self, schema, seen): >> + # Check that the members of this type do not cause duplicate JSON= fields, >> + # and update seen to track the members seen so far. Report any er= rors >> + # on behalf of info, which is not necessarily self.info >=20 > Do we actually need info !=3D self.info? If yes, test case? If no, > should the comment be adjusted? The comment is correct. For a flat union, if we have a variant associated with type 'Foo', and type 'Foo' has no clashes itself, but one of its members duplicates a member already present from the base of union, then we are calling Foo.check_clash(schema, Union.info, seen), so that the error message points to the location of Union (rather than Foo) as the place in the .json file introducing the clash. flat-union-clash-member.json tests this; it's just that we first have to remove ad hoc testing before we can trigger this case. It's on my queue, just not in this particular posting of subset C: https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg07000.html >> @@ -1035,10 +1038,13 @@ class QAPISchemaObjectTypeMember(object): >> self.type =3D schema.lookup_type(self._type_name) >> assert self.type >> >> - def check_clash(self, seen): >> - # TODO change key of seen from QAPI name to C name >> - assert self.name not in seen >> - seen[self.name] =3D self >> + def check_clash(self, info, seen): >> + name =3D c_name(self.name) >=20 > I'd call it cname. Matter of taste, of course. Sure, that works. I first tried calling it c_name, but then python thought I was trying to redefine the global c_name(). >> >> - def check_clash(self, schema, seen): >> + def check_clash(self, schema, info, seen): >> for v in self.variants: >> # Reset seen map for each variant, since qapi names from = one >> # branch do not affect another branch >> - v.type.check_clash(schema, dict(seen)) >> + v.type.check_clash(schema, info, dict(seen)) >=20 > Since we can't inherit variant members, info is always the owning objec= t > type's info. Exactly. We want to output the passed-in info, and NOT v.type.info, so that errors are reported on behalf of the union that owns the variant, rather than at the location of the variant's type which independently passed .check() and therefore has no internal collisions. And someday we might be able to inherit from objects with variants, but that's a lot further down the pipeline (if at all). >> +++ b/tests/qapi-schema/args-name-clash.json >> @@ -1,5 +1,5 @@ >> # C member name collision >> -# FIXME - This parses, but fails to compile, because the C struct is = given >> -# two 'a_b' members. Either reject this at parse time, or munge the = C names >> -# to avoid the collision. >> +# Reject members that clash when mapped to C names (we would have two= 'a_b' >> +# members). It would also be possible to munge the C names to avoid t= he >> +# collision, but unlikely to be worth the effort. >=20 > I'd drop the second sentence. Done. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --Bvs2QkgGRBNGNpM3miKHgFRgOvVO0fKP4 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/ iQEcBAEBCAAGBQJWQo0JAAoJEKeha0olJ0Nqr2IH/3azi2/fz5CCJFcGx33GsmrH 6ZFnOO7WdjxT9s5Dc3fQ3HSGgcHhOFRX11Ipf/SuttFWlOg2OfiowP9nayAZmp7w otVULTIQFVYRj2PL4QcZZdtYBCap0qauor3jqT5puZnAnaiK+w0ZYdVEvOxb9CxX R13E3CG4Jip3jVI07fF8T8aSKfpTHuem4CvDrxPmqDKlF84BfW0hHL1sqGO66JNG bvJTlH3tQHsGcNikH4xDel+4BKrRIjo1dkdJehMbMJ8kP8yjtd8PTGGyuTk9qarZ nF0h/zFypZ+ppDE2QIJMTJ/i2Ltzof3YpFy1irfIlKOPFdqIc1lG3nac+VM+nvQ= =HqRs -----END PGP SIGNATURE----- --Bvs2QkgGRBNGNpM3miKHgFRgOvVO0fKP4--