From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50973) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zlfs0-0000ce-Ms for qemu-devel@nongnu.org; Mon, 12 Oct 2015 12:22:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zlfru-00024O-Ct for qemu-devel@nongnu.org; Mon, 12 Oct 2015 12:22:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36363) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zlfru-00023a-53 for qemu-devel@nongnu.org; Mon, 12 Oct 2015 12:22:34 -0400 References: <1443930073-19359-1-git-send-email-eblake@redhat.com> <1443930073-19359-12-git-send-email-eblake@redhat.com> <8737xgqe19.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <561BDE44.4000407@redhat.com> Date: Mon, 12 Oct 2015 10:22:28 -0600 MIME-Version: 1.0 In-Reply-To: <8737xgqe19.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="5wKaXnAPX9PEeHu6Ux3Xqp2cPj3393EWE" Subject: Re: [Qemu-devel] [PATCH v7 11/14] qapi: Move duplicate member checks to schema check() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --5wKaXnAPX9PEeHu6Ux3Xqp2cPj3393EWE Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/12/2015 09:53 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> With the previous commit, we have two different locations for >> detecting member name clashes - one at parse time, and another >> at QAPISchema*.check() time. Consolidate some of the checks >> into a single place, which is also in line with our TODO to >> eventually move all of the parse time semantic checking into >> the newer schema code. The check_member_clash() function is >> no longer needed. >> >> + def check(self, schema, info, tag_type, seen, flat): >> QAPISchemaObjectTypeMember.check(self, schema, info, [], seen= ) >> assert self.name in tag_type.values >> + if flat: >> + # For flat unions, check that each QMP member does not >> + # collide with any non-variant members. No type can >> + # contain itself as a flat variant >> + self.type.check(schema) >> + assert not self.type.variants # not implemented >> + for m in self.type.members: >> + if m.c_name() in seen: >> + raise QAPIExprError(info, >> + "Member '%s' of branch '%s' c= ollides " >> + "with %s" >> + % (m.name, self.name, >> + seen[m.c_name()].describe(= ))) >=20 > If our data structures were right, we wouldn't need the conditional. >=20 Okay, you've made a good point. The only conditional that is appropriate here is 'if not alternate', because you are right that flat and simple unions should behave identically (and merely that simple unions won't detect any collisions, because flattening their implicit struct with a single 'data' member shouldn't introduce a conflict in QMP). > So far the theory, now practice: if I hack the code to test "not > alternate" (i.e. case 1 and 2) instead of "flat union" (just case 1), > "make check" passes except for union-clash-data.json. There, we now > report a clash we didn't report before: >=20 > union-clash-data.json:6: Member 'data' of branch 'data' collides wi= th 'data' (branch of TestUnion) Hmm, I'll have to investigate before posting v8. At one point, I wired in debug statements into the generator to show the contents of seen[] through each call to check(), to see where collisions are coming from; looks like I get to play with that again. >=20 > The FIXME in union-clash-data.json actually asks for an error, because > 'data' clashes with our stupid filler to avoid empty unions. But that'= s > not what this error message reports! I think what happens is this: > QAPISchemaObjectTypeVariant.check() adds the case name self.name (here:= > 'data') to seen. When we then check the case's 'data': 'int' member, i= t > finds the with the case name, and reports a clash. I can't see why it > should. >=20 > This clashing business is awfully confusing, because we have to conside= r > (flat unions, simple unions, alternates) times (QMP, C). >=20 > It would be simpler if we could make C clash only when QMP clashes. If a QMP clash always caused a C clash, life would be a bit simpler. We aren't going to get there (because in a flat union, hiding the members of the branch type behind a single pointer means those members don't clash with any C names of the overall union), but I can certainly try to make the comments explain what is going on. >=20 > We might want to separate out alternates. Dunno. And to some extent, subset C already does some of that separation (because I try to convert from 'FooKind type' to 'qtype_code type' without even creating an implicit 'FooKind' enum). It sounds like you're okay with any well-documented TODO warts related to alternates for the purposes of this subset B, especially if I can then clean those warts back up in subset C. But what v8 of subset B needs to avoid is any warts based on simple vs. flat unions. I can live with that. I'm still trying to figure out if hoisting the kind=3D>type rename into subset B makes life easier or harder (it certainly causes me more rebase churn, but that's not necessarily a bad thing). --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --5wKaXnAPX9PEeHu6Ux3Xqp2cPj3393EWE 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/ iQEcBAEBCAAGBQJWG95EAAoJEKeha0olJ0NqVNwH/i+HZmNYH+c8C/TNREYj7+q8 eVcfc5HTzPyFSxGvPiwOIRCNNIJpJUCGnr7IVW0otvRsfWDbmMmv/8oFOt+wpaoU 5XwLlLJnFD8Ry74VmfHD6b/LwNM5DatKwaj2jzdL5gjinYOeoauLwfDuNjXw735P X2FxtecSvecBjrWOd5G+kwk3Wb0qQ6b5P6ZP0I7aw71KroXTnCRPpmYo3u0HzeiX 28Flf5X7ewKN/NPYGsHRj11KB3ZwBT2xRba/1YjIwfI5ixFikvkHIuewKe/m6X8f hLlA+LdrMZKEsSPWa7Im3J3bEmfz099QOENegAZFQ+ZGvXvTSB4R+8T8EvAVYkc= =5ipH -----END PGP SIGNATURE----- --5wKaXnAPX9PEeHu6Ux3Xqp2cPj3393EWE--