Hi On Thu, Dec 6, 2018 at 8:25 PM Markus Armbruster <arm...@redhat.com> wrote: > > Marc-André Lureau <marcandre.lur...@redhat.com> writes: > > > Making a discriminator conditonal doesn't make much sense. > > Good point (so easy to overlook!), but why first add the 'if' feature > broken that way in PATCH 16, then fix it up in PATCH 18?
Not sure, I guess I found out after. Feel free to squash > > > Instead, > > the union could be made conditional. > > What do you mean by that? That the alternative is probably to make the whole union conditional > > > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> > > --- > > scripts/qapi/common.py | 11 +++++++++-- > > tests/Makefile.include | 1 + > > .../flat-union-invalid-if-discriminator.err | 1 + > > .../flat-union-invalid-if-discriminator.exit | 1 + > > .../flat-union-invalid-if-discriminator.json | 17 +++++++++++++++++ > > .../flat-union-invalid-if-discriminator.out | 0 > > 6 files changed, 29 insertions(+), 2 deletions(-) > > create mode 100644 > > tests/qapi-schema/flat-union-invalid-if-discriminator.err > > create mode 100644 > > tests/qapi-schema/flat-union-invalid-if-discriminator.exit > > create mode 100644 > > tests/qapi-schema/flat-union-invalid-if-discriminator.json > > create mode 100644 > > tests/qapi-schema/flat-union-invalid-if-discriminator.out > > > > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > > index 9b95f8cfe9..13fbb28493 100644 > > --- a/scripts/qapi/common.py > > +++ b/scripts/qapi/common.py > > @@ -577,7 +577,8 @@ def find_alternate_member_qtype(qapi_type): > > > > # Return the discriminator enum define if discriminator is specified as an > > # enum type, otherwise return None. > > -def discriminator_find_enum_define(expr): > > +def discriminator_find_enum_define(expr, info): > > + name = expr['union'] > > base = expr.get('base') > > discriminator = expr.get('discriminator') > > > > @@ -592,6 +593,11 @@ def discriminator_find_enum_define(expr): > > if not discriminator_member: > > return None > > > > + if discriminator_member.get('if'): > > + raise QAPISemError(info, 'The discriminator %s.%s for union %s ' > > + 'must not be conditional' % > > + (base, discriminator, name)) > > + > > return enum_types.get(discriminator_member['type']) > > > > > > I'm afraid this isn't the proper place to check. It's an accessor > function for @struct_types and @enum_types. The check should go into > check_union(), like this: indeed, thanks for the hint > > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index 0cf39df404..c1bc9e9c29 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -799,6 +794,11 @@ def check_union(expr, info): > "Discriminator '%s' is not a member of base " > "struct '%s'" > % (discriminator, base)) > + if discriminator_member.get('if'): > + raise QAPISemError(info, 'The discriminator %s.%s for union %s ' > + 'must not be conditional' % > + (base, discriminator, name)) > + > enum_define = enum_types.get(discriminator_member['type']) > allow_metas = ['struct'] > # Do not allow string discriminator > > > @@ -1020,7 +1026,8 @@ def check_exprs(exprs): > > > > if 'include' in expr: > > continue > > - if 'union' in expr and not discriminator_find_enum_define(expr): > > + info = expr_elem['info'] > > + if 'union' in expr and not discriminator_find_enum_define(expr, > > info): > > name = '%sKind' % expr['union'] > > elif 'alternate' in expr: > > name = '%sKind' % expr['alternate'] > > diff --git a/tests/Makefile.include b/tests/Makefile.include > > index 43e100a6cd..abc3fdf764 100644 > > --- a/tests/Makefile.include > > +++ b/tests/Makefile.include > > @@ -510,6 +510,7 @@ qapi-schema += flat-union-inline.json > > qapi-schema += flat-union-int-branch.json > > qapi-schema += flat-union-invalid-branch-key.json > > qapi-schema += flat-union-invalid-discriminator.json > > +qapi-schema += flat-union-invalid-if-discriminator.json > > qapi-schema += flat-union-no-base.json > > qapi-schema += flat-union-optional-discriminator.json > > qapi-schema += flat-union-string-discriminator.json > > diff --git a/tests/qapi-schema/flat-union-invalid-if-discriminator.err > > b/tests/qapi-schema/flat-union-invalid-if-discriminator.err > > new file mode 100644 > > index 0000000000..0c94c9860d > > --- /dev/null > > +++ b/tests/qapi-schema/flat-union-invalid-if-discriminator.err > > @@ -0,0 +1 @@ > > +tests/qapi-schema/flat-union-invalid-if-discriminator.json:13: The > > discriminator TestBase.enum1 for union TestUnion must not be conditional > > diff --git a/tests/qapi-schema/flat-union-invalid-if-discriminator.exit > > b/tests/qapi-schema/flat-union-invalid-if-discriminator.exit > > new file mode 100644 > > index 0000000000..d00491fd7e > > --- /dev/null > > +++ b/tests/qapi-schema/flat-union-invalid-if-discriminator.exit > > @@ -0,0 +1 @@ > > +1 > > diff --git a/tests/qapi-schema/flat-union-invalid-if-discriminator.json > > b/tests/qapi-schema/flat-union-invalid-if-discriminator.json > > new file mode 100644 > > index 0000000000..618ec36396 > > --- /dev/null > > +++ b/tests/qapi-schema/flat-union-invalid-if-discriminator.json > > @@ -0,0 +1,17 @@ > > +{ 'enum': 'TestEnum', > > + 'data': [ 'value1', 'value2' ] } > > + > > +{ 'struct': 'TestBase', > > + 'data': { 'enum1': { 'type': 'TestEnum', 'if': 'FOO' } } } > > + > > +{ 'struct': 'TestTypeA', > > + 'data': { 'string': 'str' } } > > + > > +{ 'struct': 'TestTypeB', > > + 'data': { 'integer': 'int' } } > > + > > +{ 'union': 'TestUnion', > > + 'base': 'TestBase', > > + 'discriminator': 'enum1', > > + 'data': { 'value1': 'TestTypeA', > > + 'value2': 'TestTypeB' } } > > diff --git a/tests/qapi-schema/flat-union-invalid-if-discriminator.out > > b/tests/qapi-schema/flat-union-invalid-if-discriminator.out > > new file mode 100644 > > index 0000000000..e69de29bb2 > -- Marc-André Lureau