Re: [Qemu-devel] [PATCH v6 19/27] qapi: add 'if' on union members

[Markus Armbruster]

In the subject, s/ on / to /.

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

> Add 'if' key to union members:
>
> { 'union': 'TestIfUnion', 'data':
>     'mem': { 'type': 'str', 'if': 'COND'} }
>
> Generated code is not changed by this patch but with "qapi: add #if
> conditions to generated code".

My suggestion on PATCH 13's commit message applies.

> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  scripts/qapi/common.py                  | 17 +++++++++--------
>  tests/qapi-schema/qapi-schema-test.json |  7 ++++++-
>  tests/qapi-schema/qapi-schema-test.out  | 10 ++++++++++
>  tests/qapi-schema/test-qapi.py          |  1 +
>  4 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 13fbb28493..e1bd9a22ba 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -813,7 +813,7 @@ def check_union(expr, info):
>      for (key, value) in members.items():
>          check_name(info, "Member of union '%s'" % name, key)
>          source = "member '%s' of union '%s'" % (key, name)
> -        check_known_keys(info, source, value, ['type'], [])
> +        check_known_keys(info, source, value, ['type'], ['if'])
>          typ = value['type']
>  
>          # Each value must name a known type
> @@ -1496,8 +1496,8 @@ class QAPISchemaObjectTypeVariants(object):
>  class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>      role = 'branch'
>  
> -    def __init__(self, name, typ):
> -        QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
> +    def __init__(self, name, typ, ifcond=None):
> +        QAPISchemaObjectTypeMember.__init__(self, name, typ, False, ifcond)
>  
>  
>  class QAPISchemaAlternateType(QAPISchemaType):
> @@ -1777,14 +1777,14 @@ class QAPISchema(object):
>      def _make_variant(self, case, typ):
>          return QAPISchemaObjectTypeVariant(case, typ)
>  
> -    def _make_simple_variant(self, case, typ, info):
> +    def _make_simple_variant(self, case, typ, ifcond, info):
>          if isinstance(typ, list):
>              assert len(typ) == 1
>              typ = self._make_array_type(typ[0], info)
>          typ = self._make_implicit_object_type(
>              typ, info, None, self.lookup_type(typ),
>              'wrapper', [self._make_member('data', typ, None, info)])
> -        return QAPISchemaObjectTypeVariant(case, typ)
> +        return QAPISchemaObjectTypeVariant(case, typ, ifcond)
>  
>      def _def_union_type(self, expr, info, doc):
>          name = expr['union']
> @@ -1802,10 +1802,11 @@ class QAPISchema(object):
>                          for (key, value) in data.items()]
>              members = []
>          else:
> -            variants = [self._make_simple_variant(key, value['type'], info)
> +            variants = [self._make_simple_variant(key, value['type'],
> +                                                  value.get('if'), info)
>                          for (key, value) in data.items()]
> -            typ = self._make_implicit_enum_type(name, info, ifcond,
> -                                                [v.name for v in variants])
> +            enum = [{'name': v.name, 'if': v.ifcond} for v in variants]
> +            typ = self._make_implicit_enum_type(name, info, ifcond, enum)
>              tag_member = QAPISchemaObjectTypeMember('type', typ, False)
>              members = [tag_member]
>          self._def_entity(
> diff --git a/tests/qapi-schema/qapi-schema-test.json 
> b/tests/qapi-schema/qapi-schema-test.json
> index 3bf440aab4..6d3c6c0b53 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -208,9 +208,14 @@
>    [ 'foo', { 'name' : 'bar', 'if': 'defined(TEST_IF_ENUM_BAR)' } ],
>    'if': 'defined(TEST_IF_ENUM)' }
>  
> -{ 'union': 'TestIfUnion', 'data': { 'foo': 'TestStruct' },
> +{ 'union': 'TestIfUnion', 'data':
> +  { 'foo': 'TestStruct',
> +    'union_bar': { 'type': 'str', 'if': 'defined(TEST_IF_UNION_BAR)'} },

"Union Bar" sounds like a cocktail lounge in northeast US :)

Back to serious: this is a simple union.  I'd prefer to test flat
unions.  Changing TestIfUnion accordingly could be done either before
this patch, or on top of this series, the latter not necessarily by
you.  Your choice.

>    'if': 'defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)' }
>  
> +{ 'command': 'TestIfUnionCmd', 'data': { 'union_cmd_arg': 'TestIfUnion' },
> +  'if': 'defined(TEST_IF_UNION)' }
> +

I'm feeling dense... why does this change belong to this patch?

>  { 'alternate': 'TestIfAlternate', 'data': { 'foo': 'int', 'bar': 
> 'TestStruct' },
>    'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }
>  
> diff --git a/tests/qapi-schema/qapi-schema-test.out 
> b/tests/qapi-schema/qapi-schema-test.out
> index 71b84878c7..ac1069cf1f 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -278,12 +278,22 @@ object q_obj_TestStruct-wrapper
>      member data: TestStruct optional=False
>  enum TestIfUnionKind
>      member foo
> +    member union_bar
> +        if ['defined(TEST_IF_UNION_BAR)']
>      if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)']
>  object TestIfUnion
>      member type: TestIfUnionKind optional=False
>      tag type
>      case foo: q_obj_TestStruct-wrapper
> +    case union_bar: q_obj_str-wrapper
> +        if ['defined(TEST_IF_UNION_BAR)']
>      if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)']
> +object q_obj_TestIfUnionCmd-arg
> +    member union_cmd_arg: TestIfUnion optional=False
> +    if ['defined(TEST_IF_UNION)']
> +command TestIfUnionCmd q_obj_TestIfUnionCmd-arg -> None
> +   gen=True success_response=True boxed=False oob=False preconfig=False
> +    if ['defined(TEST_IF_UNION)']
>  alternate TestIfAlternate
>      tag type
>      case foo: int
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 69e40d87d2..d977e936c7 100644
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -68,6 +68,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>              print('    tag %s' % variants.tag_member.name)
>              for v in variants.variants:
>                  print('    case %s: %s' % (v.name, v.type.name))
> +                QAPISchemaTestVisitor._print_if(v.ifcond, 8)
>  
>      @staticmethod
>      def _print_if(ifcond, indent=4):