From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40669) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zvrr3-0003Zw-V7 for qemu-devel@nongnu.org; Mon, 09 Nov 2015 14:11:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zvrr0-0006RY-NT for qemu-devel@nongnu.org; Mon, 09 Nov 2015 14:11:49 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36101) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zvrr0-0006RU-I6 for qemu-devel@nongnu.org; Mon, 09 Nov 2015 14:11:46 -0500 From: Markus Armbruster References: <1446791754-23823-1-git-send-email-eblake@redhat.com> <1446791754-23823-25-git-send-email-eblake@redhat.com> <87r3jzgwf3.fsf@blackfin.pond.sub.org> <5640D99D.1020006@redhat.com> Date: Mon, 09 Nov 2015 20:11:43 +0100 In-Reply-To: <5640D99D.1020006@redhat.com> (Eric Blake's message of "Mon, 9 Nov 2015 10:36:29 -0700") Message-ID: <87egfz7ztc.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v10 24/30] qapi: Factor out QAPISchemaObjectType.check_clash() 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 11/09/2015 06:00 AM, Markus Armbruster wrote: >> Eric Blake writes: >> >>> Consolidate two common sequences of clash detection into a >>> new QAPISchemaObjectType.check_clash() helper method. >>> >>> No change to generated code. >>> >>> Signed-off-by: Eric Blake >>> >>> --- > >>> @@ -980,11 +980,7 @@ class QAPISchemaObjectType(QAPISchemaType): >>> seen = OrderedDict() >>> if self._base_name: >>> self.base = schema.lookup_type(self._base_name) >>> - assert isinstance(self.base, QAPISchemaObjectType) >> >> This assertion is lost. >> >>> - assert not self.base.variants # not implemented >>> - self.base.check(schema) >>> - for m in self.base.members: >>> - m.check_clash(seen) >>> + self.base.check_clash(schema, seen) > > Directly lost, but indirectly still present. The new code is calling > QAPISchemaObjectType.check_clash(), which won't exist unless self.base > is a QAPISchemaObjectType. or a QAPISchemaObjectTypeMember, or a QAPISchemaObjectVariants, or whatever else acquires the method in the future. > Folding the assert into the refactored > function makes no sense (the condition isinstance(self, > QAPISchemaObjectType) would always be true), Correct. > and leaving the assert > prior to calling self.base.check_clash() adds no real protection against > programming bugs. Maybe, but the isinstance(self.base, QAPISchemaObjectType) will come right back anyway when we move the "'base' for FOO cannot use BAR type" check from the old semantic analysis into the check methods. Until then, it makes sense at least as a place holder. >>> @@ -1062,12 +1064,7 @@ class QAPISchemaObjectTypeVariants(object): >>> for v in self.variants: >>> # Reset seen map for each variant, since qapi names from one >>> # branch do not affect another branch >>> - vseen = dict(seen) >>> - assert isinstance(v.type, QAPISchemaObjectType) >> >> This assertion is lost. >> >>> - assert not v.type.variants # not implemented >>> - v.type.check(schema) >>> - for m in v.type.members: >>> - m.check_clash(vseen) >>> + v.type.check_clash(schema, dict(seen)) > > Same explanation.