[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-4.0 v7 15/27] qapi: add an error in case a d
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH for-4.0 v7 15/27] qapi: add an error in case a discriminator is conditional |
Date: |
Tue, 11 Dec 2018 16:23:51 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Two more nits...
Markus Armbruster <address@hidden> writes:
> Marc-André Lureau <address@hidden> 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 <address@hidden>
>> ---
>> 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.
- [Qemu-devel] [PATCH for-4.0 v7 11/27] qapi: pass long form enum to make_enum_members, (continued)
- [Qemu-devel] [PATCH for-4.0 v7 11/27] qapi: pass long form enum to make_enum_members, Marc-André Lureau, 2018/12/08
- [Qemu-devel] [PATCH for-4.0 v7 12/27] qapi: rename allow_dict to allow_implicit, Marc-André Lureau, 2018/12/08
- [Qemu-devel] [PATCH for-4.0 v7 13/27] qapi: add a dictionary form for TYPE, Marc-André Lureau, 2018/12/08
- [Qemu-devel] [PATCH for-4.0 v7 14/27] qapi: add 'if' to implicit struct members, Marc-André Lureau, 2018/12/08
- [Qemu-devel] [PATCH for-4.0 v7 15/27] qapi: add an error in case a discriminator is conditional, Marc-André Lureau, 2018/12/08
- [Qemu-devel] [PATCH for-4.0 v7 16/27] qapi: add 'if' to union members, Marc-André Lureau, 2018/12/08
- [Qemu-devel] [PATCH for-4.0 v7 17/27] qapi: simplify make_enum_members(), Marc-André Lureau, 2018/12/08
- [Qemu-devel] [PATCH for-4.0 v7 18/27] tests/qapi: add command with condition on union argument, Marc-André Lureau, 2018/12/08
- [Qemu-devel] [PATCH for-4.0 v7 19/27] qapi: add 'if' to alternate members, Marc-André Lureau, 2018/12/08