From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42157) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adLiT-0008F4-Hi for qemu-devel@nongnu.org; Tue, 08 Mar 2016 12:46:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adLiQ-0007ps-9D for qemu-devel@nongnu.org; Tue, 08 Mar 2016 12:46:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54910) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adLiQ-0007p7-4M for qemu-devel@nongnu.org; Tue, 08 Mar 2016 12:46:38 -0500 From: Markus Armbruster References: <1457194595-16189-1-git-send-email-eblake@redhat.com> <1457194595-16189-2-git-send-email-eblake@redhat.com> <87ziu9b7ns.fsf@blackfin.pond.sub.org> <56DEF4A0.8030707@redhat.com> Date: Tue, 08 Mar 2016 18:46:35 +0100 In-Reply-To: <56DEF4A0.8030707@redhat.com> (Eric Blake's message of "Tue, 8 Mar 2016 08:49:52 -0700") Message-ID: <87fuw050ck.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v4 01/10] qapi: Assert in places where variants are not handled 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: > On 03/08/2016 03:12 AM, Markus Armbruster wrote: >> Eric Blake writes: >> >>> We are getting closer to the point where we could use one union >>> as the base or variant type within another union type (as long >>> as there are no collisions between any possible combination of >>> member names allowed across all discriminator choices). But >>> until we get to that point, it is worth asserting that variants >>> are not present in places where we are not prepared to handle >>> them: base types must still be plain structs, and anywhere we >>> explode a struct into a parameter list (events and command >>> marshalling), we don't support variants in that explosion. >>> >>> Signed-off-by: Eric Blake >>> > >>> +++ b/scripts/qapi.py >>> @@ -960,6 +960,7 @@ class QAPISchemaObjectType(QAPISchemaType): >>> assert isinstance(self.base, QAPISchemaObjectType) >>> self.base.check(schema) >>> self.base.check_clash(schema, self.info, seen) >>> + assert not self.base.variants >> >> I'd move this two lines up, so it's next to the isinstance. >> >> Assertions in .check() are place-holders for semantic checks that >> haven't been moved from the old semantic analysis to the classes. >> Whenever we add one, we should double-check the old semantic analysis >> catches whatever we assert. For object types, that's check_struct() and >> check_union(). Both check_type() the base with allow_metas=['struct']), >> so we're good. >> >> Inconsistency: you add the check for base, but not for variants. >> >> On closer look, adding it for either is actually redundant, because >> se.f.base.check_clash() already asserts it, with a nice "not >> implemented" comment. >> >> If we think asserting twice is useful for base, then it's useful for >> variants, too. But I think asserting once suffices. > > So basically, we can drop this hunk, right? Yes.