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

[Marc-André Lureau]

Hi

On Mon, Dec 11, 2017 at 11:36 AM, Markus Armbruster <address@hidden> wrote:
> 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.
>

ok, changed

>>  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):
>



-- 
Marc-André Lureau