Message ID | 20181213123724.4866-15-marcandre.lureau@redhat.com |
---|---|
State | New |
Headers | show |
Series | qapi: add #if pre-processor conditions to generated code (part 2) | expand |
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > Making a discriminator conditional doesn't make much sense. "qapi: > add a dictionary form for TYPE" allows it, this patch fixes > it. Eventually the two could be merged, but for clarity of review it > is kept separate. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> I may accept the offer to let me squash it into the previous patch when I apply.
Markus Armbruster <armbru@redhat.com> writes: > Marc-André Lureau <marcandre.lureau@redhat.com> writes: > >> Making a discriminator conditional doesn't make much sense. "qapi: I guess it could be made to work as long as the discriminator's condition implies each variant's condition. Example: { 'union': 'BlockdevQcow2Encryption', 'base': { 'format': { 'type': 'BlockdevQcow2EncryptionFormat', 'if': 'defined(CONFIG_AES) || defined(CONFIG_LUKS)' } }, 'discriminator': 'format', 'data': { 'aes': { 'type': 'QCryptoBlockOptionsQCow', 'if': 'defined(CONFIG_AES)' }, 'luks': 'type': 'QCryptoBlockOptionsLUKS', 'if': 'defined(CONFIG_LUKS)' } } Now, this example looks like a bad idea. Whether there's a more legitimate use remains to be seen. Until then, outlawing conditional discriminators is totally fine with me. >> add a dictionary form for TYPE" allows it, this patch fixes >> it. Eventually the two could be merged, but for clarity of review it >> is kept separate. >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > > I may accept the offer to let me squash it into the previous patch when > I apply. Squashed patch's commit message: qapi: Add 'if' to implicit struct members The generated code is for now *unconditional*. Later patches generate the conditionals. Note that union discriminators may not have 'if' conditionals. Avoids passing judgement :)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index cc2842bcc7..8ea1dde03b 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -784,6 +784,10 @@ def check_union(expr, info): "Discriminator '%s' is not a member of base " "struct '%s'" % (discriminator, base)) + if discriminator_value.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_value['type']) allow_metas = ['struct'] # Do not allow string discriminator 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. "qapi: add a dictionary form for TYPE" allows it, this patch fixes it. Eventually the two could be merged, but for clarity of review it is kept separate. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- scripts/qapi/common.py | 4 ++++ 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, 24 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