From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60623) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ztbby-0003n7-0d for qemu-devel@nongnu.org; Tue, 03 Nov 2015 08:26:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ztbbt-0000qO-CZ for qemu-devel@nongnu.org; Tue, 03 Nov 2015 08:26:53 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51302) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ztbbt-0000qH-2m for qemu-devel@nongnu.org; Tue, 03 Nov 2015 08:26:49 -0500 References: <871tc8tnsl.fsf@blackfin.pond.sub.org> <1446504092-29008-2-git-send-email-eblake@redhat.com> <87611jjq98.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <5638B613.2020402@redhat.com> Date: Tue, 3 Nov 2015 06:26:43 -0700 MIME-Version: 1.0 In-Reply-To: <87611jjq98.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="2JLbkNjAxXQKgnJAP5nNxSapE5BMjWQIl" Subject: Re: [Qemu-devel] [PATCH v8.5 1/4] qapi: Drop all_members parameter from check() 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) --2JLbkNjAxXQKgnJAP5nNxSapE5BMjWQIl Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/03/2015 04:06 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> The implementation of QAPISchemaObjectTypeMember.check() always >> adds the member currently being checked to both the all_members >> and seen parameters. >=20 > QAPISchemaObjectTypeMember.check() does four things: >=20 > 1. Compute self.type >=20 > Precondition: all types are defined. Correct, unchanged by this patch. >=20 > 2. Accumulate members >=20 > all_members serves as accumulator. >=20 > We'll see that its only actual use is the owning object type's > check(), which uses it to compute self.members. This patch changes it to use seen.values(), which (once you use an OrderedDict() instead of plain {}) is identical to all_members. >=20 > 3. Check for collisions >=20 > This works by accumulating names in seen. Precondition: seen > contains the names seen so far. >=20 > Note that this part uses seen like a set. See 4. Unchanged by this patch; but see also 2/4 and 3/4. >=20 > 4. Accumulate a map from names to members >=20 > seen serves as accumulator. >=20 Unchanged by this patch. > We'll see that its only actual user is the owning object type's > variants.check(), which uses it to compute variants.tag_member from > variants.tag_name. >=20 >> However, the three callers of this method >> pass in the following parameters: >> >> QAPISchemaObjectType.check(): >> - all_members contains all non-variant members seen to date, >> for use in populating self.members >> - seen contains all non-variant members seen to date, for >> use in checking for collisions >=20 > Yes, and: >=20 > - we're calling it for m in self.local_members > - before the loop, all_members and seen are initialized to the inherite= d > non-variant members > - after the loop, they therefore contain all non-variant members >=20 > This caller uses all four things done by QAPISchemaObjectType.check(): >=20 > 1. Compute m.type Unchanged by this patch. > 2. Accumulate non-variant members Whether the accumulation is done via all_members (pre-patch) or by seen.values() (post-patch), this step is still done. > 3. Check for collisions among non-variant members > Before the loop, seen contains the inherited members, which don't > collide (self.base.check() ensures that). The loop adds the local > members one by one, checking for collisions. Unchanged by this patch. > 4. Accumulate a map from names to non-variant members > Similar argument to 3. Unchanged by this patch. >=20 >> QAPISchemaObjectTypeVariant.check(): >=20 > Do you mean QAPISchemaObjectVariants.check()? QAPISchemaObjectTypeVariants.check() calls QAPISchemaObjectTypeVariant.check() for each variant, but with a fresh copy of seen. We'll later need to expand this copy of seen (patch 2/4), but for this patch its use is unchanged - we are appending a single value (the tag value) which is wrong, but no one cares that we appended it because it was a copy. Patch 3/4 fixes to not append to it. >=20 >> - all_members is a throwaway empty list >> - seen is a throwaway dictionary created as a copy by >> QAPISchemaObjectVariants.check() (since the members of >> one variant cannot collide with those from another), for >> use in checking for collisions (technically, we no longer >> need to check for collisions between tag values and QMP >> key names, but that's a cleanup for another patch) >> >> QAPISchemaAlternateType.check(): >> - all_members is a throwaway empty list >> - seen is a throwaway empty dict >=20 > I'm afraid you're omitting a few steps here, and I think you missed > QAPISchemaObjectVariants.check()'s self.tag_member.check(). There is no self.tag_member.check(), any more; rather, my earlier patches have reworked things so that tag_member is checked by the owner (a flat union's base type, a simple union's local_members, or directly in QAPISchemaAlternateType prior to calling Variants.check()). I guess that's a pitfall of seeing this patch without my rework of 5/17 to address your comments there. >> Therefore, in the one case where we care about all_members >> after seen has been populated, we know that it contains the >> same members as seen.values(); changing seen to be an >> OrderedDict() is sufficient to pick up this information with >> one less parameter being passed around. >=20 > I believe the first step should be dropping the obsolete check for > collision of tag value with non-variant members. I believe this should= > do: >=20 > @@ -1059,8 +1059,7 @@ class QAPISchemaObjectTypeVariants(object): > self.tag_member.check(schema, members, seen) > assert isinstance(self.tag_member.type, QAPISchemaEnumType) > for v in self.variants: > - vseen =3D dict(seen) > - v.check(schema, self.tag_member.type, vseen) > + v.check(schema, self.tag_member.type, {}) Close, but not quite. It should do: + cases =3D {} for v in self.variants: vseen =3D dict(seen) - v.check(schema, self.tag_member.type, vseen) + v.check(schema, self.tag_member.type, vseen, cases) coupled with this in QAPISchemaObjectTypeVariant: - def check(self, schema, tag_type, seen): - QAPISchemaObjectTypeMember.check(self, schema, [], seen) + def check(self, schema, tag_type, seen, cases): + QAPISchemaObjectTypeMember.check(self, schema, [], cases) so that we are now checking collisions between tag values, rather than cases. But that's what I did in patch 3/4. And we still need seen passed to Variant.check(), because that's the checking added in 2/4. Okay, you've convinced me - when I post v9, I'll reorder these four patches to put 3/4 first. > =20 > =20 > class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): >=20 > Then only one caller about 2-4., namely QAPISchemaObjectType.check(). > Simplify radically: move 2-4. to the caller that cares, drop parameters= > all_members and seen. Nope - because seen (well, a copy of seen) is still important to patch 2/= 4. >=20 > Still to do then: non-variant member collision checking. Factor out > 3. into a helper function, use it for non-variant members. Factoring into a helper function is done in 4/4. I can try and rearrange that earlier, too. >=20 > What do you think? >=20 Can you at least look at 2, 3, and 4 to see where I'm headed, and then I can rearrange things for the v9 spin? We're probably talking a bit past each other, with the same end goal, but a muddle in the middle of how to get there. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --2JLbkNjAxXQKgnJAP5nNxSapE5BMjWQIl 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/ iQEcBAEBCAAGBQJWOLYTAAoJEKeha0olJ0Nq0L4IAId5ZBDcsSDG3TSSVFGR0Vnc EB3i3upqeDJ38ioXcymkDlY41qQ4pc9zQ/T+zL3p5NTpRSrWdQkAigmety1heTaY Oec0xaFcuP5Q9b+odGNU7NiC7scI0HRGtOpDQYYEUy78m3Jv5Ssukmf12XqeuNH2 NI61x7bA3pZlilF/r0WyKkcS1T3iR5JrDrGTzQO6wYk08lgGuJLDf2UBJjUQ97sX Rbpn3ZG+tnchm+Fsd0nI6ZLNSU2dkJcqN7a4Rd/vV2QEbnCOTIKAPKWVgl19sJEn L9QxfI2dsRu8WuNkVAkU7gVQ2N/Y+NtZhPc6y1Q4Ee/rXrdQVJU05uk7I1nK5Xc= =10B4 -----END PGP SIGNATURE----- --2JLbkNjAxXQKgnJAP5nNxSapE5BMjWQIl--