| Message ID | 20181208111606.8505-16-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Hi, | expand |
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > Making a discriminator conditional doesn't make much sense. Good point (so easy to overlook!), but why first add the 'if' feature broken that way in PATCH 13, then fix it up in PATCH 15? > Instead, > the union could be made conditional. What do you mean by that? > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > scripts/qapi/common.py | 8 ++++++++ > 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, 28 insertions(+) > 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 aec51acb07..f79b3fad54 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -578,6 +578,7 @@ 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): > + name = expr['union'] > base = expr.get('base') > discriminator = expr.get('discriminator') > > @@ -594,6 +595,7 @@ def discriminator_find_enum_define(expr): > > if isinstance(discriminator_value, dict): > discriminator_value = discriminator_value['type'] > + > return enum_types.get(discriminator_value) > > These two hunks are leftovers, please drop. > @@ -800,7 +802,12 @@ def check_union(expr, info): > "struct '%s'" > % (discriminator, base)) > if isinstance(discriminator_value, dict): > + if discriminator_value.get('if'): > + raise QAPISemError(info, 'The discriminator %s.%s for union %s ' > + 'must not be conditional' % > + (base, discriminator, name)) > discriminator_value = discriminator_value['type'] > + > enum_define = enum_types.get(discriminator_value) > allow_metas = ['struct'] > # Do not allow string discriminator > @@ -1023,6 +1030,7 @@ def check_exprs(exprs): > > if 'include' in expr: > continue > + info = expr_elem['info'] > if 'union' in expr and not discriminator_find_enum_define(expr): > name = '%sKind' % expr['union'] > elif 'alternate' in expr: > diff --git a/tests/Makefile.include b/tests/Makefile.include > index ea5d1e8787..3f5a1d0c30 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -409,6 +409,7 @@ qapi-schema += flat-union-inline-invalid-dict.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 Patch looks good otherwise.
Two more nits... Markus Armbruster <armbru@redhat.com> writes: > Marc-André Lureau <marcandre.lureau@redhat.com> writes: > >> Making a discriminator conditional doesn't make much sense. > > Good point (so easy to overlook!), but why first add the 'if' feature > broken that way in PATCH 13, then fix it up in PATCH 15? > >> Instead, >> the union could be made conditional. > > What do you mean by that? > >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> --- >> scripts/qapi/common.py | 8 ++++++++ >> 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, 28 insertions(+) >> 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 aec51acb07..f79b3fad54 100644 >> --- a/scripts/qapi/common.py >> +++ b/scripts/qapi/common.py >> @@ -578,6 +578,7 @@ 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): >> + name = expr['union'] Dead assignment. >> base = expr.get('base') >> discriminator = expr.get('discriminator') >> >> @@ -594,6 +595,7 @@ def discriminator_find_enum_define(expr): >> >> if isinstance(discriminator_value, dict): >> discriminator_value = discriminator_value['type'] >> + >> return enum_types.get(discriminator_value) >> >> > > These two hunks are leftovers, please drop. > >> @@ -800,7 +802,12 @@ def check_union(expr, info): >> "struct '%s'" >> % (discriminator, base)) >> if isinstance(discriminator_value, dict): >> + if discriminator_value.get('if'): >> + raise QAPISemError(info, 'The discriminator %s.%s for union %s ' pycodestyle complains scripts/qapi/common.py:806:80: E501 line too long (80 > 79 characters) >> + 'must not be conditional' % >> + (base, discriminator, name)) >> discriminator_value = discriminator_value['type'] >> + >> enum_define = enum_types.get(discriminator_value) >> allow_metas = ['struct'] >> # Do not allow string discriminator >> @@ -1023,6 +1030,7 @@ def check_exprs(exprs): >> >> if 'include' in expr: >> continue >> + info = expr_elem['info'] >> if 'union' in expr and not discriminator_find_enum_define(expr): >> name = '%sKind' % expr['union'] >> elif 'alternate' in expr: >> diff --git a/tests/Makefile.include b/tests/Makefile.include >> index ea5d1e8787..3f5a1d0c30 100644 >> --- a/tests/Makefile.include >> +++ b/tests/Makefile.include >> @@ -409,6 +409,7 @@ qapi-schema += flat-union-inline-invalid-dict.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 > > Patch looks good otherwise.
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index aec51acb07..f79b3fad54 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -578,6 +578,7 @@ 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): + name = expr['union'] base = expr.get('base') discriminator = expr.get('discriminator') @@ -594,6 +595,7 @@ def discriminator_find_enum_define(expr): if isinstance(discriminator_value, dict): discriminator_value = discriminator_value['type'] + return enum_types.get(discriminator_value) @@ -800,7 +802,12 @@ def check_union(expr, info): "struct '%s'" % (discriminator, base)) if isinstance(discriminator_value, dict): + if discriminator_value.get('if'): + raise QAPISemError(info, 'The discriminator %s.%s for union %s ' + 'must not be conditional' % + (base, discriminator, name)) discriminator_value = discriminator_value['type'] + enum_define = enum_types.get(discriminator_value) allow_metas = ['struct'] # Do not allow string discriminator @@ -1023,6 +1030,7 @@ def check_exprs(exprs): if 'include' in expr: continue + info = expr_elem['info'] if 'union' in expr and not discriminator_find_enum_define(expr): name = '%sKind' % expr['union'] elif 'alternate' in expr: diff --git a/tests/Makefile.include b/tests/Makefile.include index ea5d1e8787..3f5a1d0c30 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -409,6 +409,7 @@ qapi-schema += flat-union-inline-invalid-dict.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' } }
Making a discriminator conditional doesn't make much sense. Instead, the union could be made conditional. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- scripts/qapi/common.py | 8 ++++++++ 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, 28 insertions(+) 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/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