From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36881) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zu3Jj-0002c3-EW for qemu-devel@nongnu.org; Wed, 04 Nov 2015 14:01:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zu3Jf-0008Eh-8y for qemu-devel@nongnu.org; Wed, 04 Nov 2015 14:01:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36807) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zu3Jf-0008Eb-2k for qemu-devel@nongnu.org; Wed, 04 Nov 2015 14:01:51 -0500 From: Markus Armbruster References: <1446618049-13596-1-git-send-email-eblake@redhat.com> <1446618049-13596-20-git-send-email-eblake@redhat.com> Date: Wed, 04 Nov 2015 20:01:47 +0100 In-Reply-To: <1446618049-13596-20-git-send-email-eblake@redhat.com> (Eric Blake's message of "Tue, 3 Nov 2015 23:20:41 -0700") Message-ID: <87a8qtd1wk.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v9 19/27] qapi: Check for qapi collisions of flat union branches List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Michael Roth Eric Blake writes: > Right now, our ad hoc parser ensures that we cannot have a > flat union that introduces any qapi member names that would > conflict with the non-variant qapi members already present > from the union's base type (see flat-union-clash-member.json). > We want QAPISchema*.check() to make the same check, so we can > later reduce some of the ad hoc checks. > > We already ensure that all branches of a flat union are qapi > structs with no variants, at which point those members appear > in the same JSON object as all non-variant members. And we > already have a map 'seen' of all non-variant members passed > in to QAPISchemaObjectTypeVariants.check(), which we clone for > each particular variant (since the members of one variant do > not clash with another). So all that is additionally needed > is to actually check the each member of the variant type do > not add any collisions. > > In general, a type used as a branch of a flat union cannot > also be the base type of the flat union, so even though we are > adding a call to variant.type.check() in order to populate > variant.type.members, this is merely a case of gaining > topological sorting of how types are visited (and type.check() > is already set up to allow multiple calls due to base types). > > For simple unions, the same code happens to work by design, > because of our synthesized wrapper classes (however, the > wrapper has a single member 'data' which will never collide > with the one non-variant member 'type', so it doesn't really > matter). > > But for alternates, we do NOT want to check the type members > for adding collisions (an alternate has no parent JSON object > that is merging in member names, the way a flat union does), so > we branch on whether seen is empty to distinguish whether we > are checking a union or an alternate. > > No change to generated code. > > Signed-off-by: Eric Blake > > --- > v9: new patch, split off from v8 7/17 > --- > scripts/qapi.py | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index a20abda..3054628 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -1057,8 +1057,9 @@ class QAPISchemaObjectTypeVariants(object): > assert self.tag_member in seen.itervalues() > assert isinstance(self.tag_member.type, QAPISchemaEnumType) > for v in self.variants: > - vseen = dict(seen) > - v.check(schema, self.tag_member.type, vseen) > + # Reset seen array for each variant, since qapi names from one > + # branch do not affect another branch > + v.check(schema, self.tag_member.type, dict(seen)) > > > class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): > @@ -1068,6 +1069,14 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): > def check(self, schema, tag_type, seen): > QAPISchemaObjectTypeMember.check(self, schema) > assert self.name in tag_type.values > + if seen: > + # This variant is used within a union; ensure each qapi member > + # field does not collide with the union's non-variant members. > + assert isinstance(self.type, QAPISchemaObjectType) > + assert not self.type.variants # not implemented > + self.type.check(schema) > + for m in self.type.members: > + m.check_clash(seen) > > # This function exists to support ugly simple union special cases > # TODO get rid of them, and drop the function Two call chains: * QAPISchemaObjectType.check() via QAPISchemaObjectTypeVariants.check() In this case, the new conditional executes. * QAPISchemaAlternateType.check() via same In this case, it doesn't. Why can't we simply add the new code to QAPISchemaObjectType.check()? The rest of the clash checking is already there...