[{"id":1818419,"web_url":"http://patchwork.ozlabs.org/comment/1818419/","msgid":"<87r2s5ipgd.fsf@dusky.pond.sub.org>","list_archive_url":null,"date":"2017-12-08T08:38:58","subject":"Re: [Qemu-devel] [PATCH v3 19/50] qapi: add 'if' to enum members","submitter":{"id":2645,"url":"http://patchwork.ozlabs.org/api/people/2645/","name":"Markus Armbruster","email":"armbru@redhat.com"},"content":"A bit more detail in the commit message would make this patch easier to\nreview.\n\nMarc-André Lureau <marcandre.lureau@redhat.com> writes:\n\n> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>\n> ---\n>  scripts/qapi.py                         | 39 ++++++++++++++++++++++++++++-----\n>  tests/Makefile.include                  |  1 -\n>  tests/qapi-schema/enum-dict-member.err  |  1 -\n>  tests/qapi-schema/enum-dict-member.exit |  1 -\n>  tests/qapi-schema/enum-dict-member.json |  2 --\n>  tests/qapi-schema/enum-dict-member.out  |  0\n>  tests/qapi-schema/qapi-schema-test.json |  5 +++--\n>  tests/qapi-schema/qapi-schema-test.out  |  3 ++-\n>  tests/qapi-schema/test-qapi.py          |  2 ++\n>  9 files changed, 41 insertions(+), 13 deletions(-)\n>  delete mode 100644 tests/qapi-schema/enum-dict-member.err\n>  delete mode 100644 tests/qapi-schema/enum-dict-member.exit\n>  delete mode 100644 tests/qapi-schema/enum-dict-member.json\n>  delete mode 100644 tests/qapi-schema/enum-dict-member.out\n>\n> diff --git a/scripts/qapi.py b/scripts/qapi.py\n> index 386a577a59..1535de9ce7 100644\n> --- a/scripts/qapi.py\n> +++ b/scripts/qapi.py\n> @@ -659,6 +659,14 @@ def check_if(expr, info):\n>              info, \"'if' condition must be a string or a list of strings\")\n>  \n>  \n> +def check_unknown_keys(info, dict, allowed_keys):\n> +    diff = set(dict) - allowed_keys\n> +    if not diff:\n> +        return\n> +    raise QAPISemError(info, \"Dictionnary has unknown keys: %s (allowed: %s)\" %\n\ns/Dictionnary/Dictionary/\n\n> +        (', '.join(diff), ', '.join(allowed_keys)))\n\nI'm afraid this duplicates a part of check_keys().  Could it be factored\nout?\n\n> +\n> +\n>  def check_type(info, source, value, allow_array=False,\n>                 allow_dict=False, allow_optional=False,\n>                 allow_metas=[]):\n> @@ -739,6 +747,10 @@ def check_event(expr, info):\n>                 allow_metas=meta)\n>  \n>  \n> +def enum_get_values(expr):\n> +    return [e if isinstance(e, str) else e['name'] for e in expr['data']]\n> +\n> +\n>  def check_union(expr, info):\n>      name = expr['union']\n>      base = expr.get('base')\n> @@ -798,7 +810,7 @@ def check_union(expr, info):\n>          # If the discriminator names an enum type, then all members\n>          # of 'data' must also be members of the enum type.\n>          if enum_define:\n> -            if key not in enum_define['data']:\n> +            if key not in enum_get_values(enum_define):\n>                  raise QAPISemError(info,\n>                                     \"Discriminator value '%s' is not found in \"\n>                                     \"enum '%s'\"\n> @@ -806,7 +818,7 @@ def check_union(expr, info):\n>  \n>      # If discriminator is user-defined, ensure all values are covered\n>      if enum_define:\n> -        for value in enum_define['data']:\n> +        for value in enum_get_values(enum_define):\n>              if value not in members.keys():\n>                  raise QAPISemError(info, \"Union '%s' data missing '%s' branch\"\n>                                     % (name, value))\n> @@ -837,7 +849,7 @@ def check_alternate(expr, info):\n>          if qtype == 'QTYPE_QSTRING':\n>              enum_expr = enum_types.get(value)\n>              if enum_expr:\n> -                for v in enum_expr['data']:\n> +                for v in enum_get_values(enum_expr):\n>                      if v in ['on', 'off']:\n>                          conflicting.add('QTYPE_QBOOL')\n>                      if re.match(r'[-+0-9.]', v): # lazy, could be tightened\n> @@ -865,6 +877,14 @@ def check_enum(expr, info):\n>          raise QAPISemError(info,\n>                             \"Enum '%s' requires a string for 'prefix'\" % name)\n>      for member in members:\n> +        if isinstance(member, dict):\n> +            if 'name' not in member:\n> +                raise QAPISemError(info, \"Dictionary member of enum '%s' must \"\n> +                                   \"have a 'name' key\" % name)\n> +            if 'if' in member:\n> +                check_if(member, info)\n> +            check_unknown_keys(info, member, {'name', 'if'})\n> +            member = member['name']\n>          check_name(info, \"Member of enum '%s'\" % name, member,\n>                     enum_member=True)\n>  \n> @@ -1280,9 +1300,11 @@ class QAPISchemaObjectType(QAPISchemaType):\n>  class QAPISchemaMember(object):\n>      role = 'member'\n>  \n> -    def __init__(self, name):\n> +    def __init__(self, name, ifcond=None):\n>          assert isinstance(name, str)\n> +        assert ifcond is None or isinstance(ifcond, str)\n>          self.name = name\n> +        self.ifcond = ifcond\n>          self.owner = None\n>  \n>      def set_owner(self, name):\n\nQAPISchemaObjectTypeMember inherits .ifcond.  Peeking ahead: looks like\nit'll get used when we add conditions to object type members, PATCH\n23ff.  Okay.  Mentioning it in the commit message wouldn't hurt, though.\n\n> @@ -1559,7 +1581,14 @@ class QAPISchema(object):\n>                                              qtype_values, 'QTYPE'))\n>  \n>      def _make_enum_members(self, values):\n> -        return [QAPISchemaMember(v) for v in values]\n> +        enum = []\n> +        for v in values:\n> +            ifcond = None\n> +            if isinstance(v, dict):\n> +                ifcond = v.get('if')\n> +                v = v['name']\n> +            enum.append(QAPISchemaMember(v, ifcond))\n\n\nI like brevity a lot, but if it's bought by assigning to a loop control\nvariable, I pass.  Cleaner:\n\n           for v in values:\n               if isinstance(v, dict):\n                   name = v['name']\n                   ifcond = v.get('if')\n               else:\n                   name = v\n                   ifcond = None\n           enum.append(QAPISchemaMember(name, ifcond))\n                   \n> +        return enum\n>  \n>      def _make_implicit_enum_type(self, name, info, ifcond, values):\n>          # See also QAPISchemaObjectTypeMember._pretty_owner()\n> diff --git a/tests/Makefile.include b/tests/Makefile.include\n> index 8dac7c9083..a9f0ddbe01 100644\n> --- a/tests/Makefile.include\n> +++ b/tests/Makefile.include\n> @@ -443,7 +443,6 @@ qapi-schema += empty.json\n>  qapi-schema += enum-bad-name.json\n>  qapi-schema += enum-bad-prefix.json\n>  qapi-schema += enum-clash-member.json\n> -qapi-schema += enum-dict-member.json\n>  qapi-schema += enum-int-member.json\n>  qapi-schema += enum-member-case.json\n>  qapi-schema += enum-missing-data.json\n> diff --git a/tests/qapi-schema/enum-dict-member.err b/tests/qapi-schema/enum-dict-member.err\n> deleted file mode 100644\n> index 8ca146ea59..0000000000\n> --- a/tests/qapi-schema/enum-dict-member.err\n> +++ /dev/null\n> @@ -1 +0,0 @@\n> -tests/qapi-schema/enum-dict-member.json:2: Member of enum 'MyEnum' requires a string name\n> diff --git a/tests/qapi-schema/enum-dict-member.exit b/tests/qapi-schema/enum-dict-member.exit\n> deleted file mode 100644\n> index d00491fd7e..0000000000\n> --- a/tests/qapi-schema/enum-dict-member.exit\n> +++ /dev/null\n> @@ -1 +0,0 @@\n> -1\n> diff --git a/tests/qapi-schema/enum-dict-member.json b/tests/qapi-schema/enum-dict-member.json\n> deleted file mode 100644\n> index 79672e0f09..0000000000\n> --- a/tests/qapi-schema/enum-dict-member.json\n> +++ /dev/null\n> @@ -1,2 +0,0 @@\n> -# we reject any enum member that is not a string\n> -{ 'enum': 'MyEnum', 'data': [ { 'value': 'str' } ] }\n> diff --git a/tests/qapi-schema/enum-dict-member.out b/tests/qapi-schema/enum-dict-member.out\n> deleted file mode 100644\n> index e69de29bb2..0000000000\n\nHmm.  The dict instance of \"enum value must be of an appropriate JSON\ntype\" error class goes away in this patch, but the class remains.  Do we\nstill cover it?\n\nThere's enum-int-member.json, but it's not a good test: it dies in the\nlexer.  I think we should replace the two tests by a single one.\nPerhaps something like 'data': [ [] ] would do.\n\n> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json\n> index dc2c444fc1..ad2b405d83 100644\n> --- a/tests/qapi-schema/qapi-schema-test.json\n> +++ b/tests/qapi-schema/qapi-schema-test.json\n> @@ -194,7 +194,8 @@\n>  { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },\n>    'if': 'defined(TEST_IF_STRUCT)' }\n>  \n> -{ 'enum': 'TestIfEnum', 'data': [ 'foo', 'bar' ],\n> +{ 'enum': 'TestIfEnum', 'data':\n> +  [ 'foo', { 'name' : 'bar', 'if': 'defined(TEST_IF_ENUM_BAR)' } ],\n>    'if': 'defined(TEST_IF_ENUM)' }\n>  \n>  { 'union': 'TestIfUnion', 'data': { 'foo': 'TestStruct' },\n> @@ -203,7 +204,7 @@\n>  { 'alternate': 'TestIfAlternate', 'data': { 'foo': 'int', 'bar': 'TestStruct' },\n>    'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }\n>  \n> -{ 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct' },\n> +{ 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct', 'bar': 'TestIfEnum' },\n>    'if': 'defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT)' }\n>  \n>  { 'event': 'TestIfEvent', 'data': { 'foo': 'TestIfStruct' },\n\nShould this hunk be in PATCH 04?\n\n> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out\n> index 9a7cafc269..8a0cf1a551 100644\n> --- a/tests/qapi-schema/qapi-schema-test.out\n> +++ b/tests/qapi-schema/qapi-schema-test.out\n> @@ -74,7 +74,7 @@ command TestIfCmd q_obj_TestIfCmd-arg -> None\n>      if defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT)\n>  enum TestIfEnum\n>      member foo:\n> -    member bar:\n> +    member bar: if=defined(TEST_IF_ENUM_BAR)\n>      if defined(TEST_IF_ENUM)\n>  event TestIfEvent q_obj_TestIfEvent-arg\n>     boxed=False\n> @@ -228,6 +228,7 @@ object q_obj_EVENT_D-arg\n>      member enum3: EnumOne optional=True\n>  object q_obj_TestIfCmd-arg\n>      member foo: TestIfStruct optional=False\n> +    member bar: TestIfEnum optional=False\n>      if defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT)\n>  object q_obj_TestIfEvent-arg\n>      member foo: TestIfStruct optional=False\n> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py\n> index 67c6c1ecef..a86c3b5ee1 100644\n> --- a/tests/qapi-schema/test-qapi.py\n> +++ b/tests/qapi-schema/test-qapi.py\n> @@ -56,6 +56,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):\n>              print '    member %s:' % m.name,\n>              if isinstance(m, QAPISchemaObjectTypeMember):\n>                  print '%s optional=%s' % (m.type.name, m.optional),\n> +            if m.ifcond:\n> +                print 'if=%s' % m.ifcond,\n>              print\n>  \n>      @staticmethod","headers":{"Return-Path":"<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>","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=<UNKNOWN>)","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 3ytQl14cGFz9s71\n\tfor <incoming@patchwork.ozlabs.org>;\n\tFri,  8 Dec 2017 19:39:40 +1100 (AEDT)","from localhost ([::1]:36051 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>)\n\tid 1eNEC1-0000aL-Ol\n\tfor incoming@patchwork.ozlabs.org; Fri, 08 Dec 2017 03:39:37 -0500","from eggs.gnu.org ([2001:4830:134:3::10]:42590)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from <armbru@redhat.com>) id 1eNEBZ-0000Z6-24\n\tfor qemu-devel@nongnu.org; Fri, 08 Dec 2017 03:39:10 -0500","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from <armbru@redhat.com>) id 1eNEBU-0002ps-HA\n\tfor qemu-devel@nongnu.org; Fri, 08 Dec 2017 03:39:09 -0500","from mx1.redhat.com ([209.132.183.28]:58372)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from <armbru@redhat.com>) id 1eNEBU-0002pi-7P\n\tfor qemu-devel@nongnu.org; Fri, 08 Dec 2017 03:39:04 -0500","from smtp.corp.redhat.com\n\t(int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])\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 31628356E5;\n\tFri,  8 Dec 2017 08:39:03 +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 50B9C1964A;\n\tFri,  8 Dec 2017 08:39:00 +0000 (UTC)","by blackfin.pond.sub.org (Postfix, from userid 1000)\n\tid D5F2B1138658; Fri,  8 Dec 2017 09:38:58 +0100 (CET)"],"From":"Markus Armbruster <armbru@redhat.com>","To":"=?utf-8?q?Marc-Andr=C3=A9?= Lureau <marcandre.lureau@redhat.com>","References":"<20170911110623.24981-1-marcandre.lureau@redhat.com>\n\t<20170911110623.24981-20-marcandre.lureau@redhat.com>","Date":"Fri, 08 Dec 2017 09:38:58 +0100","In-Reply-To":"<20170911110623.24981-20-marcandre.lureau@redhat.com> (\n\t=?utf-8?b?Ik1hcmMtQW5kcsOp?= Lureau\"'s message of \"Mon,\n\t11 Sep 2017 \t13:05:52 +0200\")","Message-ID":"<87r2s5ipgd.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.11","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.30]);\n\tFri, 08 Dec 2017 08:39:03 +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 19/50] qapi: add 'if' to enum members","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"<qemu-devel.nongnu.org>","List-Unsubscribe":"<https://lists.nongnu.org/mailman/options/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>","List-Archive":"<http://lists.nongnu.org/archive/html/qemu-devel/>","List-Post":"<mailto:qemu-devel@nongnu.org>","List-Help":"<mailto:qemu-devel-request@nongnu.org?subject=help>","List-Subscribe":"<https://lists.nongnu.org/mailman/listinfo/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=subscribe>","Cc":"qemu-devel@nongnu.org, Michael Roth <mdroth@linux.vnet.ibm.com>","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>"}},{"id":1837333,"web_url":"http://patchwork.ozlabs.org/comment/1837333/","msgid":"<CAJ+F1CLdM92EkdPDw=UL65G476dMMUNMVaLd3hi-66Mk9N99Jg@mail.gmail.com>","list_archive_url":null,"date":"2018-01-11T21:24:22","subject":"Re: [Qemu-devel] [PATCH v3 19/50] qapi: add 'if' to enum members","submitter":{"id":6442,"url":"http://patchwork.ozlabs.org/api/people/6442/","name":"Marc-André Lureau","email":"marcandre.lureau@gmail.com"},"content":"Hi\n\nOn Fri, Dec 8, 2017 at 9:38 AM, Markus Armbruster <armbru@redhat.com> wrote:\n> A bit more detail in the commit message would make this patch easier to\n> review.\n>\n> Marc-André Lureau <marcandre.lureau@redhat.com> writes:\n>\n>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>\n>> ---\n>>  scripts/qapi.py                         | 39 ++++++++++++++++++++++++++++-----\n>>  tests/Makefile.include                  |  1 -\n>>  tests/qapi-schema/enum-dict-member.err  |  1 -\n>>  tests/qapi-schema/enum-dict-member.exit |  1 -\n>>  tests/qapi-schema/enum-dict-member.json |  2 --\n>>  tests/qapi-schema/enum-dict-member.out  |  0\n>>  tests/qapi-schema/qapi-schema-test.json |  5 +++--\n>>  tests/qapi-schema/qapi-schema-test.out  |  3 ++-\n>>  tests/qapi-schema/test-qapi.py          |  2 ++\n>>  9 files changed, 41 insertions(+), 13 deletions(-)\n>>  delete mode 100644 tests/qapi-schema/enum-dict-member.err\n>>  delete mode 100644 tests/qapi-schema/enum-dict-member.exit\n>>  delete mode 100644 tests/qapi-schema/enum-dict-member.json\n>>  delete mode 100644 tests/qapi-schema/enum-dict-member.out\n>>\n>> diff --git a/scripts/qapi.py b/scripts/qapi.py\n>> index 386a577a59..1535de9ce7 100644\n>> --- a/scripts/qapi.py\n>> +++ b/scripts/qapi.py\n>> @@ -659,6 +659,14 @@ def check_if(expr, info):\n>>              info, \"'if' condition must be a string or a list of strings\")\n>>\n>>\n>> +def check_unknown_keys(info, dict, allowed_keys):\n>> +    diff = set(dict) - allowed_keys\n>> +    if not diff:\n>> +        return\n>> +    raise QAPISemError(info, \"Dictionnary has unknown keys: %s (allowed: %s)\" %\n>\n> s/Dictionnary/Dictionary/\n\nok\n\n>\n>> +        (', '.join(diff), ', '.join(allowed_keys)))\n>\n> I'm afraid this duplicates a part of check_keys().  Could it be factored\n> out?\n\ndone\n\n>\n>> +\n>> +\n>>  def check_type(info, source, value, allow_array=False,\n>>                 allow_dict=False, allow_optional=False,\n>>                 allow_metas=[]):\n>> @@ -739,6 +747,10 @@ def check_event(expr, info):\n>>                 allow_metas=meta)\n>>\n>>\n>> +def enum_get_values(expr):\n>> +    return [e if isinstance(e, str) else e['name'] for e in expr['data']]\n>> +\n>> +\n>>  def check_union(expr, info):\n>>      name = expr['union']\n>>      base = expr.get('base')\n>> @@ -798,7 +810,7 @@ def check_union(expr, info):\n>>          # If the discriminator names an enum type, then all members\n>>          # of 'data' must also be members of the enum type.\n>>          if enum_define:\n>> -            if key not in enum_define['data']:\n>> +            if key not in enum_get_values(enum_define):\n>>                  raise QAPISemError(info,\n>>                                     \"Discriminator value '%s' is not found in \"\n>>                                     \"enum '%s'\"\n>> @@ -806,7 +818,7 @@ def check_union(expr, info):\n>>\n>>      # If discriminator is user-defined, ensure all values are covered\n>>      if enum_define:\n>> -        for value in enum_define['data']:\n>> +        for value in enum_get_values(enum_define):\n>>              if value not in members.keys():\n>>                  raise QAPISemError(info, \"Union '%s' data missing '%s' branch\"\n>>                                     % (name, value))\n>> @@ -837,7 +849,7 @@ def check_alternate(expr, info):\n>>          if qtype == 'QTYPE_QSTRING':\n>>              enum_expr = enum_types.get(value)\n>>              if enum_expr:\n>> -                for v in enum_expr['data']:\n>> +                for v in enum_get_values(enum_expr):\n>>                      if v in ['on', 'off']:\n>>                          conflicting.add('QTYPE_QBOOL')\n>>                      if re.match(r'[-+0-9.]', v): # lazy, could be tightened\n>> @@ -865,6 +877,14 @@ def check_enum(expr, info):\n>>          raise QAPISemError(info,\n>>                             \"Enum '%s' requires a string for 'prefix'\" % name)\n>>      for member in members:\n>> +        if isinstance(member, dict):\n>> +            if 'name' not in member:\n>> +                raise QAPISemError(info, \"Dictionary member of enum '%s' must \"\n>> +                                   \"have a 'name' key\" % name)\n>> +            if 'if' in member:\n>> +                check_if(member, info)\n>> +            check_unknown_keys(info, member, {'name', 'if'})\n>> +            member = member['name']\n>>          check_name(info, \"Member of enum '%s'\" % name, member,\n>>                     enum_member=True)\n>>\n>> @@ -1280,9 +1300,11 @@ class QAPISchemaObjectType(QAPISchemaType):\n>>  class QAPISchemaMember(object):\n>>      role = 'member'\n>>\n>> -    def __init__(self, name):\n>> +    def __init__(self, name, ifcond=None):\n>>          assert isinstance(name, str)\n>> +        assert ifcond is None or isinstance(ifcond, str)\n>>          self.name = name\n>> +        self.ifcond = ifcond\n>>          self.owner = None\n>>\n>>      def set_owner(self, name):\n>\n> QAPISchemaObjectTypeMember inherits .ifcond.  Peeking ahead: looks like\n> it'll get used when we add conditions to object type members, PATCH\n> 23ff.  Okay.  Mentioning it in the commit message wouldn't hurt, though.\n\nIt will *also* get used. Ok, updated commit message.\n\n>\n>> @@ -1559,7 +1581,14 @@ class QAPISchema(object):\n>>                                              qtype_values, 'QTYPE'))\n>>\n>>      def _make_enum_members(self, values):\n>> -        return [QAPISchemaMember(v) for v in values]\n>> +        enum = []\n>> +        for v in values:\n>> +            ifcond = None\n>> +            if isinstance(v, dict):\n>> +                ifcond = v.get('if')\n>> +                v = v['name']\n>> +            enum.append(QAPISchemaMember(v, ifcond))\n>\n>\n> I like brevity a lot, but if it's bought by assigning to a loop control\n> variable, I pass.  Cleaner:\n>\n>            for v in values:\n>                if isinstance(v, dict):\n>                    name = v['name']\n>                    ifcond = v.get('if')\n>                else:\n>                    name = v\n>                    ifcond = None\n>            enum.append(QAPISchemaMember(name, ifcond))\n>\n\nsure\n\n>> +        return enum\n>>\n>>      def _make_implicit_enum_type(self, name, info, ifcond, values):\n>>          # See also QAPISchemaObjectTypeMember._pretty_owner()\n>> diff --git a/tests/Makefile.include b/tests/Makefile.include\n>> index 8dac7c9083..a9f0ddbe01 100644\n>> --- a/tests/Makefile.include\n>> +++ b/tests/Makefile.include\n>> @@ -443,7 +443,6 @@ qapi-schema += empty.json\n>>  qapi-schema += enum-bad-name.json\n>>  qapi-schema += enum-bad-prefix.json\n>>  qapi-schema += enum-clash-member.json\n>> -qapi-schema += enum-dict-member.json\n>>  qapi-schema += enum-int-member.json\n>>  qapi-schema += enum-member-case.json\n>>  qapi-schema += enum-missing-data.json\n>> diff --git a/tests/qapi-schema/enum-dict-member.err b/tests/qapi-schema/enum-dict-member.err\n>> deleted file mode 100644\n>> index 8ca146ea59..0000000000\n>> --- a/tests/qapi-schema/enum-dict-member.err\n>> +++ /dev/null\n>> @@ -1 +0,0 @@\n>> -tests/qapi-schema/enum-dict-member.json:2: Member of enum 'MyEnum' requires a string name\n>> diff --git a/tests/qapi-schema/enum-dict-member.exit b/tests/qapi-schema/enum-dict-member.exit\n>> deleted file mode 100644\n>> index d00491fd7e..0000000000\n>> --- a/tests/qapi-schema/enum-dict-member.exit\n>> +++ /dev/null\n>> @@ -1 +0,0 @@\n>> -1\n>> diff --git a/tests/qapi-schema/enum-dict-member.json b/tests/qapi-schema/enum-dict-member.json\n>> deleted file mode 100644\n>> index 79672e0f09..0000000000\n>> --- a/tests/qapi-schema/enum-dict-member.json\n>> +++ /dev/null\n>> @@ -1,2 +0,0 @@\n>> -# we reject any enum member that is not a string\n>> -{ 'enum': 'MyEnum', 'data': [ { 'value': 'str' } ] }\n>> diff --git a/tests/qapi-schema/enum-dict-member.out b/tests/qapi-schema/enum-dict-member.out\n>> deleted file mode 100644\n>> index e69de29bb2..0000000000\n>\n> Hmm.  The dict instance of \"enum value must be of an appropriate JSON\n> type\" error class goes away in this patch, but the class remains.  Do we\n> still cover it?\n>\n> There's enum-int-member.json, but it's not a good test: it dies in the\n> lexer.  I think we should replace the two tests by a single one.\n> Perhaps something like 'data': [ [] ] would do.\n\nI replaced enum-dict-member by enum-bad-member. I left the int-member\ntest, because it has also the purpose to check the lexer.\n\n>\n>> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json\n>> index dc2c444fc1..ad2b405d83 100644\n>> --- a/tests/qapi-schema/qapi-schema-test.json\n>> +++ b/tests/qapi-schema/qapi-schema-test.json\n>> @@ -194,7 +194,8 @@\n>>  { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },\n>>    'if': 'defined(TEST_IF_STRUCT)' }\n>>\n>> -{ 'enum': 'TestIfEnum', 'data': [ 'foo', 'bar' ],\n>> +{ 'enum': 'TestIfEnum', 'data':\n>> +  [ 'foo', { 'name' : 'bar', 'if': 'defined(TEST_IF_ENUM_BAR)' } ],\n>>    'if': 'defined(TEST_IF_ENUM)' }\n>>\n>>  { 'union': 'TestIfUnion', 'data': { 'foo': 'TestStruct' },\n>> @@ -203,7 +204,7 @@\n>>  { 'alternate': 'TestIfAlternate', 'data': { 'foo': 'int', 'bar': 'TestStruct' },\n>>    'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }\n>>\n>> -{ 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct' },\n>> +{ 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct', 'bar': 'TestIfEnum' },\n>>    'if': 'defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT)' }\n>>\n>>  { 'event': 'TestIfEvent', 'data': { 'foo': 'TestIfStruct' },\n>\n> Should this hunk be in PATCH 04?\n\nIt could, but it is not necessary until conditionals are added to\nverify introspection generation.\n\n>\n>> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out\n>> index 9a7cafc269..8a0cf1a551 100644\n>> --- a/tests/qapi-schema/qapi-schema-test.out\n>> +++ b/tests/qapi-schema/qapi-schema-test.out\n>> @@ -74,7 +74,7 @@ command TestIfCmd q_obj_TestIfCmd-arg -> None\n>>      if defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT)\n>>  enum TestIfEnum\n>>      member foo:\n>> -    member bar:\n>> +    member bar: if=defined(TEST_IF_ENUM_BAR)\n>>      if defined(TEST_IF_ENUM)\n>>  event TestIfEvent q_obj_TestIfEvent-arg\n>>     boxed=False\n>> @@ -228,6 +228,7 @@ object q_obj_EVENT_D-arg\n>>      member enum3: EnumOne optional=True\n>>  object q_obj_TestIfCmd-arg\n>>      member foo: TestIfStruct optional=False\n>> +    member bar: TestIfEnum optional=False\n>>      if defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT)\n>>  object q_obj_TestIfEvent-arg\n>>      member foo: TestIfStruct optional=False\n>> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py\n>> index 67c6c1ecef..a86c3b5ee1 100644\n>> --- a/tests/qapi-schema/test-qapi.py\n>> +++ b/tests/qapi-schema/test-qapi.py\n>> @@ -56,6 +56,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):\n>>              print '    member %s:' % m.name,\n>>              if isinstance(m, QAPISchemaObjectTypeMember):\n>>                  print '%s optional=%s' % (m.type.name, m.optional),\n>> +            if m.ifcond:\n>> +                print 'if=%s' % m.ifcond,\n>>              print\n>>\n>>      @staticmethod\n>","headers":{"Return-Path":"<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>","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=<UNKNOWN>)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"VbwNxqNm\"; dkim-atps=neutral"],"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 3zHfCD4VS4z9sNr\n\tfor <incoming@patchwork.ozlabs.org>;\n\tFri, 12 Jan 2018 08:29:12 +1100 (AEDT)","from localhost ([::1]:44001 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>)\n\tid 1eZkPO-0008Uq-Lc\n\tfor incoming@patchwork.ozlabs.org; Thu, 11 Jan 2018 16:29:10 -0500","from eggs.gnu.org ([2001:4830:134:3::10]:44766)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from <marcandre.lureau@gmail.com>) id 1eZkKp-0004j4-G1\n\tfor qemu-devel@nongnu.org; Thu, 11 Jan 2018 16:24:29 -0500","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from <marcandre.lureau@gmail.com>) id 1eZkKn-0001DA-JR\n\tfor qemu-devel@nongnu.org; Thu, 11 Jan 2018 16:24:27 -0500","from mail-wr0-x244.google.com ([2a00:1450:400c:c0c::244]:35564)\n\tby eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16)\n\t(Exim 4.71) (envelope-from <marcandre.lureau@gmail.com>)\n\tid 1eZkKn-0001CW-8s\n\tfor qemu-devel@nongnu.org; Thu, 11 Jan 2018 16:24:25 -0500","by mail-wr0-x244.google.com with SMTP id g38so344934wrd.2\n\tfor <qemu-devel@nongnu.org>; Thu, 11 Jan 2018 13:24:25 -0800 (PST)","by 10.223.199.143 with HTTP; Thu, 11 Jan 2018 13:24:22 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=tFUwRcwMK+ZcEjOrmem3KFTnUOB2vG8KGKcsmB08/2U=;\n\tb=VbwNxqNmOF0ObUE6e8WZW7VBkM+2nM79746ElOfjyLNPo541sI27HA9UpUPyVKalQM\n\tUiHT/AvI0ggXDejxeyg/KI2GOOdAGj6UTV6bBzIzZI7kVx/fnElsV1dker5dBTaPLAvm\n\t3fihI5UW37Qt6GgGTe3va+Na/iMzK3GCQFk2DMxnhwGPiMzvR57V5pu1tU8H+Vh6VL3e\n\t8a7mhAmTaY2RXqPrqZWdcBoQjhABC/R2Kk0AApsP3YB9k6iGY21zAIRLEXkR+aWvkjsV\n\tP3/b2QveXVXzlduZJpk9n/5f499hKbQknn4MGjC0fB1TuXHnc2dRyZk6bK4amtIoq7+C\n\tOlKA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=tFUwRcwMK+ZcEjOrmem3KFTnUOB2vG8KGKcsmB08/2U=;\n\tb=D8lnY2bW5R2Uvqy9frWyeJb/TBBfpYVzAV8OBbjlEwh2cHcP8sjMwg9A6WfvCHYo9w\n\t01iF5ZHULmkxqMoEXAPhw9jogXmgVRDchxozfDjSoz/FStEX1b+UrsLTcUGf/ksneZ6b\n\tuikJU8e409+n2y/zyffIA+WrEkgP7v/hmx/OpnFVjkb8R+DS2sbzGOXaZMVvx/iUQ2q4\n\ti6A7Edoxzhz9NQwofyLXMeYn/I42Zhj9pX0+YVXGCCr9e0DGbNFd51a6VpUOW8NVFiM5\n\taEtzVHR31CQN8SDHEQb2EWZjZ7Y9mlQAkPlpD5A486KgMXbwnOjRNTtNozzc3cnULyR2\n\tUa2Q==","X-Gm-Message-State":"AKGB3mLTRJFJjyGeIfsXneXIEgVkm0uUfL5cPa5BCBnZkSPh26yKgcj9\n\tMAC4stPSvlQ2stDy2FdZ5bRa7h9KL0o/M0N/has=","X-Google-Smtp-Source":"ACJfBouq+7Hb4qfQyk8gGjC024SXvSEUBiNBE8IUrVFcesHV7aU+dM38JHPotAL82q75dQ+1RN+qOaKSreNdZzBYvNs=","X-Received":"by 10.223.155.131 with SMTP id d3mr20595260wrc.134.1515705864022;\n\tThu, 11 Jan 2018 13:24:24 -0800 (PST)","MIME-Version":"1.0","In-Reply-To":"<87r2s5ipgd.fsf@dusky.pond.sub.org>","References":"<20170911110623.24981-1-marcandre.lureau@redhat.com>\n\t<20170911110623.24981-20-marcandre.lureau@redhat.com>\n\t<87r2s5ipgd.fsf@dusky.pond.sub.org>","From":"=?utf-8?q?Marc-Andr=C3=A9_Lureau?= <marcandre.lureau@gmail.com>","Date":"Thu, 11 Jan 2018 22:24:22 +0100","Message-ID":"<CAJ+F1CLdM92EkdPDw=UL65G476dMMUNMVaLd3hi-66Mk9N99Jg@mail.gmail.com>","To":"Markus Armbruster <armbru@redhat.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","X-detected-operating-system":"by eggs.gnu.org: Genre and OS details not\n\trecognized.","X-Received-From":"2a00:1450:400c:c0c::244","Subject":"Re: [Qemu-devel] [PATCH v3 19/50] qapi: add 'if' to enum members","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"<qemu-devel.nongnu.org>","List-Unsubscribe":"<https://lists.nongnu.org/mailman/options/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>","List-Archive":"<http://lists.nongnu.org/archive/html/qemu-devel/>","List-Post":"<mailto:qemu-devel@nongnu.org>","List-Help":"<mailto:qemu-devel-request@nongnu.org?subject=help>","List-Subscribe":"<https://lists.nongnu.org/mailman/listinfo/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=subscribe>","Cc":"QEMU <qemu-devel@nongnu.org>, Michael Roth <mdroth@linux.vnet.ibm.com>","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>"}}]