From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39238) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbTRQ-0003mF-Hz for qemu-devel@nongnu.org; Fri, 27 Mar 2015 08:32:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YbTRN-0003rl-7E for qemu-devel@nongnu.org; Fri, 27 Mar 2015 08:32:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38947) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbTRN-0003rd-31 for qemu-devel@nongnu.org; Fri, 27 Mar 2015 08:32:45 -0400 From: Markus Armbruster References: <1427227433-5030-1-git-send-email-eblake@redhat.com> <1427227433-5030-11-git-send-email-eblake@redhat.com> <87d23v24uv.fsf@blackfin.pond.sub.org> <5514252C.7040901@redhat.com> Date: Fri, 27 Mar 2015 13:32:42 +0100 In-Reply-To: <5514252C.7040901@redhat.com> (Eric Blake's message of "Thu, 26 Mar 2015 09:26:36 -0600") Message-ID: <87bnjelixh.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v5 10/28] qapi: Segregate anonymous unions into alternates in generator List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, wenchaoqemu@gmail.com, lcapitulino@redhat.com Eric Blake writes: > On 03/26/2015 08:47 AM, Markus Armbruster wrote: >> Eric Blake writes: >> >>> Special-casing 'discriminator == {}' for handling anonymous unions >>> is getting awkward; since this particular type is not always a >>> dictionary on the wire, it is easier to treat it as a completely >>> different class of type, "alternate", so that if a type is listed >>> in the union_types array, we know it is not an anonymous union. >>> >>> This patch just further segregates union handling, to make sure that >>> anonymous unions are not stored in union_types, and splitting up >>> check_union() into separate functions. A future patch will change >>> the qapi grammar, and having the segregation already in place will >>> make it easier to deal with the distinct meta-type. >>> >>> Signed-off-by: Eric Blake >>> --- > >>> @@ -535,7 +546,8 @@ def find_struct(name): >>> >>> def add_union(definition): >>> global union_types >>> - union_types.append(definition) >>> + if definition.get('discriminator') != {}: >>> + union_types.append(definition) >>> >>> def find_union(name): >>> global union_types >> >> This is the only unobvious hunk. >> >> union_types is used only through find_union. The hunk makes >> find_union(N) return None when N names an anonymous union. >> >> find_union() is used in two places: >> >> * find_alternate_member_qtype() >> >> Patched further up. It really wants only non-anonymous unions, and >> this change to find_union() renders its check for anonymous unions >> superfluous. Good. >> >> * generate_visit_alternate() >> >> Asserts that each member's type is either a built-in type, a complex >> type, a union type, or an enum type. >> >> The change relaxes the assertion not to trigger on anonymous union >> types. Why is that okay? > > No, the change tightens the assertion so that it will now fail on an > anonymous union nested as a branch of another anonymous union (where > before it could silently pass the assertion), because the anonymous > union is no longer found by find_union(). And this is okay because the > earlier change to find_alternate_member_qtype means that we don't allow > nested anonymous unions, so making the assertion fail if an anonymous > union gets through anyway is correct. Thanks for unconfusing me. Reviewed-by: Markus Armbruster