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

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

Hi
On Thu, Dec 6, 2018 at 8:37 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> In the subject, s/ on / to /.
>
> Marc-André Lureau <marcandre.lureau@redhat.com> 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.

ok

>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  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.

I prefer to defer

>
> >    '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?

Hmm, no strong reason, I'll make it a separate commit.
>
> >  { '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):