Re: [Qemu-devel] [PATCH v3 26/50] qapi: add 'if' on union variants

[Markus Armbruster]

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

> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  scripts/qapi.py                         | 15 ++++++++++-----
>  tests/qapi-schema/qapi-schema-test.json |  7 ++++++-
>  tests/qapi-schema/qapi-schema-test.out  |  8 ++++++++
>  tests/qapi-schema/test-qapi.py          |  5 ++++-
>  4 files changed, 28 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 15711f96fa..2f14edfa1f 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1412,8 +1412,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):
> @@ -1674,13 +1674,18 @@ class QAPISchema(object):
>          return QAPISchemaObjectTypeVariant(case, typ)
>  
>      def _make_simple_variant(self, case, typ, info):
> +        ifcond = None
> +        if isinstance(typ, dict):
> +            check_unknown_keys(info, typ, {'type', 'if'})
> +            ifcond = typ.get('if')
> +            typ = typ['type']
>          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).ifcond,
>              'wrapper', [self._make_member('data', typ, info)])
> -        return QAPISchemaObjectTypeVariant(case, typ)
> +        return QAPISchemaObjectTypeVariant(case, typ, ifcond)
>  
>      def _def_union_type(self, expr, info, doc):
>          name = expr['union']
> @@ -1700,8 +1705,8 @@ class QAPISchema(object):
>          else:
>              variants = [self._make_simple_variant(key, value, info)
>                          for (key, value) in data.iteritems()]
> -            typ = self._make_implicit_enum_type(name, info, ifcond,
> -                                                [v.name for v in variants])
> +            values = [{'name': v.name, 'if': v.ifcond} for v in variants]
> +            typ = self._make_implicit_enum_type(name, info, ifcond, values)
>              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 5cfccabb3d..895e80a978 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -200,9 +200,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)'} },
>    'if': 'defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)' }
>  
> +{ 'command': 'TestIfUnionCmd', 'data': { 'union_cmd_arg': 'TestIfUnion' },
> +  'if': 'defined(TEST_IF_UNION)' }
> +
>  { '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 6df4e49c69..ee009c5626 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -87,9 +87,14 @@ 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)

PATCH 22, but I only spotted it here.  We say "if=COND" in some places,
...

>      if defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)
> +command TestIfUnionCmd q_obj_TestIfUnionCmd-arg -> None
> +   gen=True success_response=True boxed=False
> +    if defined(TEST_IF_UNION)

... and "if COND" in other places.  I guess the '=' is there to match
existing flag printing like optional=BOOL.

I'd prefer less decorated output, i.e. instead of

    enum TestIfEnum
        member foo:
        member bar: if=defined(TEST_IF_ENUM_BAR)
        if defined(TEST_IF_ENUM)

something like

enum TestIfEnum
    member foo
    member bar
        if defined(TEST_IF_ENUM_BAR)
    if defined(TEST_IF_ENUM)

Could touch that up on commit.

>  enum TestIfUnionKind
>      member foo:
> +    member union_bar: if=defined(TEST_IF_UNION_BAR)
>      if defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)
>  object TestStruct
>      member integer: int optional=False
> @@ -235,6 +240,9 @@ object q_obj_TestIfEvent-arg
>      member foo: TestIfStruct optional=False
>      member bar: TestIfEnum optional=False if=defined(TEST_IF_EVT_BAR)
>      if defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)
> +object q_obj_TestIfUnionCmd-arg
> +    member union_cmd_arg: TestIfUnion optional=False
> +    if defined(TEST_IF_UNION)
>  object q_obj_TestStruct-wrapper
>      member data: TestStruct optional=False
>  object q_obj_UserDefFlatUnion2-base
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index a86c3b5ee1..87a8efff78 100644
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -65,7 +65,10 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>          if variants:
>              print '    tag %s' % variants.tag_member.name
>              for v in variants.variants:
> -                print '    case %s: %s' % (v.name, v.type.name)
> +                print '    case %s: %s' % (v.name, v.type.name),
> +                if v.ifcond:
> +                    print 'if=%s' % v.ifcond,
> +                print
>  
>      @staticmethod
>      def _print_if(ifcond):