Re: [Qemu-devel] [PATCH v6 13/27] qapi: add 'if' to enum members

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> QAPISchemaMember gains .ifcond for enum members: inherited classes,
> such as QAPISchemaObjectTypeMember, will thus have an ifcond member
> after this (those different types will also use the .ifcond to store
> the condition and generate conditional code in the following patches).
>
> Generated code is not changed by this patch, but with "qapi: add #if
> conditions to generated code" patch.

It's actually "qapi: add #if conditions to generated code members".  I'd
sidestep this like you did in commit 967c885108f

    The generated code is for now *unconditional*. Later patches generate
    the conditionals.

except I'd say "The generated code remains unconditional for now."

> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  scripts/qapi/common.py                         | 10 +++++++---
>  tests/Makefile.include                         |  1 +
>  tests/qapi-schema/enum-dict-member-unknown.err |  2 +-
>  tests/qapi-schema/enum-if-invalid.err          |  1 +
>  tests/qapi-schema/enum-if-invalid.exit         |  1 +
>  tests/qapi-schema/enum-if-invalid.json         |  3 +++
>  tests/qapi-schema/enum-if-invalid.out          |  0
>  tests/qapi-schema/qapi-schema-test.json        |  5 +++--
>  tests/qapi-schema/qapi-schema-test.out         |  2 ++
>  tests/qapi-schema/test-qapi.py                 |  1 +
>  10 files changed, 20 insertions(+), 6 deletions(-)
>  create mode 100644 tests/qapi-schema/enum-if-invalid.err
>  create mode 100644 tests/qapi-schema/enum-if-invalid.exit
>  create mode 100644 tests/qapi-schema/enum-if-invalid.json
>  create mode 100644 tests/qapi-schema/enum-if-invalid.out
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index e9fb736d46..95b7cd74ee 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -877,7 +877,8 @@ def check_enum(expr, info):
>  
>      for member in members:
>          source = "dictionary member of enum '%s'" % name
> -        check_known_keys(info, source, member, ['name'], [])
> +        check_known_keys(info, source, member, ['name'], ['if'])
> +        check_if(member, info)
>          check_name(info, "Member of enum '%s'" % name, member['name'],
>                     enum_member=True)
>  
> @@ -1357,9 +1358,10 @@ class QAPISchemaObjectType(QAPISchemaType):
>  class QAPISchemaMember(object):
>      role = 'member'
>  
> -    def __init__(self, name):
> +    def __init__(self, name, ifcond=None):
>          assert isinstance(name, str)
>          self.name = name
> +        self.ifcond = listify_cond(ifcond)
>          self.owner = None
>  
>      def set_owner(self, name):
> @@ -1670,9 +1672,11 @@ class QAPISchema(object):
>          for v in values:
>              if isinstance(v, dict):
>                  name = v['name']
> +                ifcond = v.get('if')
>              else:
>                  name = v
> -            enum.append(QAPISchemaMember(name))
> +                ifcond = None
> +            enum.append(QAPISchemaMember(name, ifcond))
>          return enum
>  
>      def _make_implicit_enum_type(self, name, info, ifcond, values):
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 8e1b122cf2..3ba9097892 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -487,6 +487,7 @@ qapi-schema += enum-bad-name.json
>  qapi-schema += enum-bad-prefix.json
>  qapi-schema += enum-clash-member.json
>  qapi-schema += enum-dict-member-unknown.json
> +qapi-schema += enum-if-invalid.json
>  qapi-schema += enum-int-member.json
>  qapi-schema += enum-member-case.json
>  qapi-schema += enum-missing-data.json
> diff --git a/tests/qapi-schema/enum-dict-member-unknown.err 
> b/tests/qapi-schema/enum-dict-member-unknown.err
> index 76bd0471db..2aae618be0 100644
> --- a/tests/qapi-schema/enum-dict-member-unknown.err
> +++ b/tests/qapi-schema/enum-dict-member-unknown.err
> @@ -1,2 +1,2 @@
>  tests/qapi-schema/enum-dict-member-unknown.json:2: Unknown key 'bad-key' in 
> dictionary member of enum 'MyEnum'
> -Valid keys are 'name'.
> +Valid keys are 'if', 'name'.
> diff --git a/tests/qapi-schema/enum-if-invalid.err 
> b/tests/qapi-schema/enum-if-invalid.err
> new file mode 100644
> index 0000000000..54c3cf887b
> --- /dev/null
> +++ b/tests/qapi-schema/enum-if-invalid.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/enum-if-invalid.json:2: 'if' condition must be a string or 
> a list of strings
> diff --git a/tests/qapi-schema/enum-if-invalid.exit 
> b/tests/qapi-schema/enum-if-invalid.exit
> new file mode 100644
> index 0000000000..d00491fd7e
> --- /dev/null
> +++ b/tests/qapi-schema/enum-if-invalid.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/enum-if-invalid.json 
> b/tests/qapi-schema/enum-if-invalid.json
> new file mode 100644
> index 0000000000..60bd0ef1d7
> --- /dev/null
> +++ b/tests/qapi-schema/enum-if-invalid.json
> @@ -0,0 +1,3 @@
> +# check invalid 'if' type
> +{ 'enum': 'TestIfEnum', 'data':
> +  [ 'foo', { 'name' : 'bar', 'if': { 'val': 'foo' } } ] }
> diff --git a/tests/qapi-schema/enum-if-invalid.out 
> b/tests/qapi-schema/enum-if-invalid.out
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/tests/qapi-schema/qapi-schema-test.json 
> b/tests/qapi-schema/qapi-schema-test.json
> index 11aa4c8f8d..35ca94d991 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -202,7 +202,8 @@
>  { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
>    'if': 'defined(TEST_IF_STRUCT)' }
>  
> -{ 'enum': 'TestIfEnum', 'data': [ 'foo', 'bar' ],
> +{ 'enum': 'TestIfEnum', 'data':
> +  [ 'foo', { 'name' : 'bar', 'if': 'defined(TEST_IF_ENUM_BAR)' } ],
>    'if': 'defined(TEST_IF_ENUM)' }
>  
>  { 'union': 'TestIfUnion', 'data': { 'foo': 'TestStruct' },
> @@ -211,7 +212,7 @@
>  { 'alternate': 'TestIfAlternate', 'data': { 'foo': 'int', 'bar': 
> 'TestStruct' },
>    'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }
>  
> -{ 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct' },
> +{ 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct', 'bar': 
> 'TestIfEnum' },
>    'returns': 'UserDefThree',
>    'if': ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'] }
>  
> diff --git a/tests/qapi-schema/qapi-schema-test.out 
> b/tests/qapi-schema/qapi-schema-test.out
> index edd22bc306..1fb33f302c 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -270,6 +270,7 @@ object TestIfStruct
>  enum TestIfEnum
>      member foo
>      member bar
> +        if ['defined(TEST_IF_ENUM_BAR)']
>      if ['defined(TEST_IF_ENUM)']
>  object q_obj_TestStruct-wrapper
>      member data: TestStruct optional=False
> @@ -288,6 +289,7 @@ alternate TestIfAlternate
>      if ['defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)']
>  object q_obj_TestIfCmd-arg
>      member foo: TestIfStruct optional=False
> +    member bar: TestIfEnum optional=False
>      if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)']
>  command TestIfCmd q_obj_TestIfCmd-arg -> UserDefThree
>     gen=True success_response=True boxed=False oob=False preconfig=False
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 641a18f06d..b2f4ce6134 100644
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -29,6 +29,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>              print('    prefix %s' % prefix)
>          for m in members:
>              print('    member %s' % m.name)
> +            self._print_if(m.ifcond, 8)

Could write indent=8.  Your choice.

>          self._print_if(ifcond)
>  
>      def visit_object_type(self, name, info, ifcond, base, members, variants):

Preferably with a tweaked commit message:
Reviewed-by: Markus Armbruster <address@hidden>