| Message ID | 20190910063724.28470-10-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: Schema language cleanups & doc improvements | expand |
On 9/10/19 1:37 AM, Markus Armbruster wrote: > A union or alternate without branches makes no sense and doesn't work: > it can't be instantiated. A union or alternate with just one branch > works, but is degenerate. We accept the former, but reject the > latter. Weird. docs/devel/qapi-code-gen.txt doesn't mention the > difference. It claims an alternate definition is "is similar to a > simple union type". > > Permit degenerate alternates to make them consistent with unions. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > scripts/qapi/common.py | 6 ++---- > tests/qapi-schema/alternate-empty.err | 2 +- > tests/qapi-schema/alternate-empty.json | 4 ++-- > tests/qapi-schema/qapi-schema-test.json | 4 +++- > tests/qapi-schema/qapi-schema-test.out | 6 ++++-- > 5 files changed, 12 insertions(+), 10 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com> (Might make sense if one of the branches is conditional, where depending on compile-time options it could be a two-branch or a one-branch alternate)
Eric Blake <eblake@redhat.com> writes: > On 9/10/19 1:37 AM, Markus Armbruster wrote: >> A union or alternate without branches makes no sense and doesn't work: >> it can't be instantiated. A union or alternate with just one branch >> works, but is degenerate. We accept the former, but reject the >> latter. Weird. docs/devel/qapi-code-gen.txt doesn't mention the >> difference. It claims an alternate definition is "is similar to a >> simple union type". >> >> Permit degenerate alternates to make them consistent with unions. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> scripts/qapi/common.py | 6 ++---- >> tests/qapi-schema/alternate-empty.err | 2 +- >> tests/qapi-schema/alternate-empty.json | 4 ++-- >> tests/qapi-schema/qapi-schema-test.json | 4 +++- >> tests/qapi-schema/qapi-schema-test.out | 6 ++++-- >> 5 files changed, 12 insertions(+), 10 deletions(-) > > Reviewed-by: Eric Blake <eblake@redhat.com> > > (Might make sense if one of the branches is conditional, where depending > on compile-time options it could be a two-branch or a one-branch alternate) The QAPI generator doesn't even try to reason about conditions. It happily lets you define a union or alternate with branches that are all conditional, and where the conditions are all false for a particular build configuration. Ensuring the QAPI schema is sane for all build configurations is the schema author's problem.
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 6e25479939..46a4b07a04 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -917,11 +917,9 @@ def check_alternate(expr, info): members = expr['data'] types_seen = {} - # Check every branch; require at least two branches - if len(members) < 2: + if len(members) == 0: raise QAPISemError(info, - "Alternate '%s' should have at least two branches " - "in 'data'" % name) + "Alternate '%s' cannot have empty 'data'" % name) for (key, value) in members.items(): check_name(info, "Member of alternate '%s'" % name, key) check_known_keys(info, diff --git a/tests/qapi-schema/alternate-empty.err b/tests/qapi-schema/alternate-empty.err index bb06c5bfec..86dbc666eb 100644 --- a/tests/qapi-schema/alternate-empty.err +++ b/tests/qapi-schema/alternate-empty.err @@ -1 +1 @@ -tests/qapi-schema/alternate-empty.json:2: Alternate 'Alt' should have at least two branches in 'data' +tests/qapi-schema/alternate-empty.json:2: Alternate 'Alt' cannot have empty 'data' diff --git a/tests/qapi-schema/alternate-empty.json b/tests/qapi-schema/alternate-empty.json index fff15baf16..9f445474e6 100644 --- a/tests/qapi-schema/alternate-empty.json +++ b/tests/qapi-schema/alternate-empty.json @@ -1,2 +1,2 @@ -# alternates must list at least two types to be useful -{ 'alternate': 'Alt', 'data': { 'i': 'int' } } +# alternates cannot be empty +{ 'alternate': 'Alt', 'data': { } } diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index e6dbbbd328..8b0d47c4ab 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -186,19 +186,21 @@ # test that we correctly compile downstream extensions, as well as munge # ticklish names +# also test union and alternate with just one branch { 'enum': '__org.qemu_x-Enum', 'data': [ '__org.qemu_x-value' ] } { 'struct': '__org.qemu_x-Base', 'data': { '__org.qemu_x-member1': '__org.qemu_x-Enum' } } { 'struct': '__org.qemu_x-Struct', 'base': '__org.qemu_x-Base', 'data': { '__org.qemu_x-member2': 'str', '*wchar-t': 'int' } } { 'union': '__org.qemu_x-Union1', 'data': { '__org.qemu_x-branch': 'str' } } +{ 'alternate': '__org.qemu_x-Alt1', 'data': { '__org.qemu_x-branch': 'str' } } { 'struct': '__org.qemu_x-Struct2', 'data': { 'array': ['__org.qemu_x-Union1'] } } { 'union': '__org.qemu_x-Union2', 'base': '__org.qemu_x-Base', 'discriminator': '__org.qemu_x-member1', 'data': { '__org.qemu_x-value': '__org.qemu_x-Struct2' } } { 'alternate': '__org.qemu_x-Alt', - 'data': { '__org.qemu_x-branch': 'str', 'b': '__org.qemu_x-Base' } } + 'data': { '__org.qemu_x-branch': '__org.qemu_x-Base' } } { 'event': '__ORG.QEMU_X-EVENT', 'data': '__org.qemu_x-Struct' } { 'command': '__org.qemu_x-command', 'data': { 'a': ['__org.qemu_x-Enum'], 'b': ['__org.qemu_x-Struct'], diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index fb00a21996..bea7976bbb 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -274,6 +274,9 @@ object __org.qemu_x-Union1 member type: __org.qemu_x-Union1Kind optional=False tag type case __org.qemu_x-branch: q_obj_str-wrapper +alternate __org.qemu_x-Alt1 + tag type + case __org.qemu_x-branch: str array __org.qemu_x-Union1List __org.qemu_x-Union1 object __org.qemu_x-Struct2 member array: __org.qemu_x-Union1List optional=False @@ -283,8 +286,7 @@ object __org.qemu_x-Union2 case __org.qemu_x-value: __org.qemu_x-Struct2 alternate __org.qemu_x-Alt tag type - case __org.qemu_x-branch: str - case b: __org.qemu_x-Base + case __org.qemu_x-branch: __org.qemu_x-Base event __ORG.QEMU_X-EVENT __org.qemu_x-Struct boxed=False array __org.qemu_x-EnumList __org.qemu_x-Enum
A union or alternate without branches makes no sense and doesn't work: it can't be instantiated. A union or alternate with just one branch works, but is degenerate. We accept the former, but reject the latter. Weird. docs/devel/qapi-code-gen.txt doesn't mention the difference. It claims an alternate definition is "is similar to a simple union type". Permit degenerate alternates to make them consistent with unions. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- scripts/qapi/common.py | 6 ++---- tests/qapi-schema/alternate-empty.err | 2 +- tests/qapi-schema/alternate-empty.json | 4 ++-- tests/qapi-schema/qapi-schema-test.json | 4 +++- tests/qapi-schema/qapi-schema-test.out | 6 ++++-- 5 files changed, 12 insertions(+), 10 deletions(-)