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