From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44173) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zvlzq-00018O-H4 for qemu-devel@nongnu.org; Mon, 09 Nov 2015 07:56:31 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zvlzn-0003yD-AM for qemu-devel@nongnu.org; Mon, 09 Nov 2015 07:56:30 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35924) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zvlzn-0003xj-5Q for qemu-devel@nongnu.org; Mon, 09 Nov 2015 07:56:27 -0500 From: Markus Armbruster References: <1446791754-23823-1-git-send-email-eblake@redhat.com> <1446791754-23823-24-git-send-email-eblake@redhat.com> Date: Mon, 09 Nov 2015 13:56:24 +0100 In-Reply-To: <1446791754-23823-24-git-send-email-eblake@redhat.com> (Eric Blake's message of "Thu, 5 Nov 2015 23:35:47 -0700") Message-ID: <87vb9bgwlj.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v10 23/30] 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 QAPISchemaObjectType.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. All > we need is a new QAPISchemaObjectTypeVariants.check_clash(), > which clones the seen map then checks for clashes with each > member of the variant's qapi type. > > Note that the clone of seen inside Variants.check_clash() > resembles the one we just removed from Variants.check(); the > difference here is that we are now checking for clashes > among the qapi members of the variant type, rather than for > a single clash with the variant tag name itself. > > 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). Yes, a type cannot contain itself, neither as base nor as variant. We have tests covering attempts to do the former (struct-cycle-direct.json, struct-cycle-indirect.json). As far as I can see, we don't have tests covering the latter. Do we catch it? > 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). > > There is no impact to alternates, which intentionally do not > need to call variants.check_clash() (there, at most one of > the variant's branches will be an ObjectType, and even if one > exists, we are not inlining the qapi members of that object > into a parent object, the way we do for unions). > > No change to generated code. > > Signed-off-by: Eric Blake Patch looks good.