[{"id":1817075,"web_url":"http://patchwork.ozlabs.org/comment/1817075/","msgid":"<87h8t3wzi4.fsf@dusky.pond.sub.org>","list_archive_url":null,"date":"2017-12-06T17:13:39","subject":"Re: [Qemu-devel] [PATCH v3 06/50] qapi: pass 'if' condition into\n\tQAPISchemaEntity objects","submitter":{"id":2645,"url":"http://patchwork.ozlabs.org/api/people/2645/","name":"Markus Armbruster","email":"armbru@redhat.com"},"content":"Marc-André Lureau writes:\n\n> Built-in objects remain unconditional. Explicitly defined objects\n> use the condition specified in the schema. Implicitly defined\n> objects inherit their condition from their users. For most of them,\n> there is exactly one user, so the condition to use is obvious. The\n> exception is the wrapper types generated for simple union variants,\n> which can be shared by any number of simple unions. The tight\n> condition would be the disjunction of the conditions of these simple\n> unions. For now, use wrapped type's condition instead. Much\n\nthe wrapped type's\n\n> simpler and good enough for now.\n>\n> Signed-off-by: Marc-André Lureau \n> ---\n> scripts/qapi.py | 89 ++++++++++++++++++++++++++++++++++++---------------------\n> 1 file changed, 57 insertions(+), 32 deletions(-)\n>\n> diff --git a/scripts/qapi.py b/scripts/qapi.py\n> index 20c1abf915..0f55caa18d 100644\n> --- a/scripts/qapi.py\n> +++ b/scripts/qapi.py\n> @@ -1000,7 +1000,7 @@ def check_exprs(exprs):\n> #\n> \n> class QAPISchemaEntity(object):\n> - def __init__(self, name, info, doc):\n> + def __init__(self, name, info, doc, ifcond=None):\n> assert isinstance(name, str)\n> self.name = name\n> # For explicitly defined entities, info points to the (explicit)\n> @@ -1010,6 +1010,7 @@ class QAPISchemaEntity(object):\n> # such place).\n> self.info = info\n> self.doc = doc\n> + self.ifcond = ifcond\n> \n> def c_name(self):\n> return c_name(self.name)\n> @@ -1126,8 +1127,8 @@ class QAPISchemaBuiltinType(QAPISchemaType):\n> \n> \n> class QAPISchemaEnumType(QAPISchemaType):\n> - def __init__(self, name, info, doc, values, prefix):\n> - QAPISchemaType.__init__(self, name, info, doc)\n> + def __init__(self, name, info, doc, ifcond, values, prefix):\n> + QAPISchemaType.__init__(self, name, info, doc, ifcond)\n> for v in values:\n> assert isinstance(v, QAPISchemaMember)\n> v.set_owner(name)\n> @@ -1162,7 +1163,7 @@ class QAPISchemaEnumType(QAPISchemaType):\n> \n> class QAPISchemaArrayType(QAPISchemaType):\n> def __init__(self, name, info, element_type):\n> - QAPISchemaType.__init__(self, name, info, None)\n> + QAPISchemaType.__init__(self, name, info, None, None)\n> assert isinstance(element_type, str)\n> self._element_type_name = element_type\n> self.element_type = None\n> @@ -1170,6 +1171,7 @@ class QAPISchemaArrayType(QAPISchemaType):\n> def check(self, schema):\n> self.element_type = schema.lookup_type(self._element_type_name)\n> assert self.element_type\n> + self.ifcond = self.element_type.ifcond\n> \n> def is_implicit(self):\n> return True\n\nIn my review of v2, I wrote:\n\n This is subtler than it looks on first glance.\n\n All the other entities set self.ifcond in their constructor to the true\n value passed in as argument.\n\n QAPISchemaArrayType doesn't take such an argument. Instead, it inherits\n its .ifcond from its .element_type. However, .element_type isn't known\n at construction time if it's a forward reference. We therefore delay\n setting it until .check() time. You do the same for .ifcond (no\n choice).\n\n Before .check(), .ifcond is None, because the constructor sets it that\n way: it calls QAPISchemaType.__init__() without passing a ifcond\n argument, which then sets self.ifcond to its default argument None.\n\n Pitfall: accessing ent.ifcond before ent.check() works *except* when ent\n is an array type. Hmm.\n\nThe problem is of course more general. We commonly initialize\nattributes to None in .init(), then set their real value in .check().\nAccessing the attribute before .check() yields None. If we're lucky,\nthe code that accesses the attribute prematurely chokes on None.\n\nIt won't for .ifcond, because None is a legitimate value.\n\nPerhaps we should leave such attributes undefined until .check(). What\ndo you think? No need to tie that idea to this series, though.\n\n[...]\n\nWith the commit message tidied up:\nReviewed-by: Markus Armbruster ","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3ysQFb40DGz9s71\n\tfor ;\n\tThu, 7 Dec 2017 04:14:11 +1100 (AEDT)","from localhost ([::1]:56732 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1eMdGr-0002RN-H2\n\tfor incoming@patchwork.ozlabs.org; Wed, 06 Dec 2017 12:14:09 -0500","from eggs.gnu.org ([2001:4830:134:3::10]:39703)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1eMdGX-0002R8-6S\n\tfor qemu-devel@nongnu.org; Wed, 06 Dec 2017 12:13:50 -0500","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1eMdGS-0002bw-Kh\n\tfor qemu-devel@nongnu.org; Wed, 06 Dec 2017 12:13:49 -0500","from mx1.redhat.com ([209.132.183.28]:40112)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1eMdGS-0002ZE-CR\n\tfor qemu-devel@nongnu.org; Wed, 06 Dec 2017 12:13:44 -0500","from smtp.corp.redhat.com\n\t(int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 5E814820F3;\n\tWed, 6 Dec 2017 17:13:43 +0000 (UTC)","from blackfin.pond.sub.org (ovpn-116-74.ams2.redhat.com\n\t[10.36.116.74])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 2C9425C1A3;\n\tWed, 6 Dec 2017 17:13:41 +0000 (UTC)","by blackfin.pond.sub.org (Postfix, from userid 1000)\n\tid B10681138658; Wed, 6 Dec 2017 18:13:39 +0100 (CET)"],"From":"Markus Armbruster ","To":"=?utf-8?q?Marc-Andr=C3=A9?= Lureau ","References":"<20170911110623.24981-1-marcandre.lureau@redhat.com>\n\t<20170911110623.24981-7-marcandre.lureau@redhat.com>","Date":"Wed, 06 Dec 2017 18:13:39 +0100","In-Reply-To":"<20170911110623.24981-7-marcandre.lureau@redhat.com> (\n\t=?utf-8?b?Ik1hcmMtQW5kcsOp?= Lureau\"'s message of \"Mon,\n\t11 Sep 2017 13:05:39 +0200\")","Message-ID":"<87h8t3wzi4.fsf@dusky.pond.sub.org>","User-Agent":"Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux)","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.16","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.26]);\n\tWed, 06 Dec 2017 17:13:43 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH v3 06/50] qapi: pass 'if' condition into\n\tQAPISchemaEntity objects","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"qemu-devel@nongnu.org, Michael Roth ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]