| Message ID | 1455582057-27565-3-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > Empty unions serve no purpose, and while we compile with gcc > which permits them, strict C99 forbids them. We could inject > a dummy member (and in fact, we do for empty structs), but while gen_variants() injects void *data. > empty structs make sense in qapi, Suggest to cut the paragaph until here. > empty unions don't add any > expressiveness to the QMP language. So prohibit them at parse > time. Update the documentation and testsuite to match. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v10: hoist into earlier series > [no v7, v8, or v9] > v6: rebase to earlier qapi.py cleanups > --- > scripts/qapi.py | 12 ++++++++++-- > docs/qapi-code-gen.txt | 15 ++++++++------- > tests/qapi-schema/alternate-empty.err | 1 + > tests/qapi-schema/alternate-empty.exit | 2 +- > tests/qapi-schema/alternate-empty.json | 2 +- > tests/qapi-schema/alternate-empty.out | 5 ----- > tests/qapi-schema/flat-union-empty.err | 1 + > tests/qapi-schema/flat-union-empty.exit | 2 +- > tests/qapi-schema/flat-union-empty.json | 2 +- > tests/qapi-schema/flat-union-empty.out | 9 --------- > tests/qapi-schema/union-empty.err | 1 + > tests/qapi-schema/union-empty.exit | 2 +- > tests/qapi-schema/union-empty.json | 2 +- > tests/qapi-schema/union-empty.out | 6 ------ > 14 files changed, 27 insertions(+), 35 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index f40dc9e..f97236f 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -590,7 +590,10 @@ def check_union(expr, expr_info): > "Discriminator '%s' must be of enumeration " > "type" % discriminator) > > - # Check every branch > + # Check every branch; don't allow an empty union > + if len(members) == 0: > + raise QAPIExprError(expr_info, > + "Union '%s' cannot have empty 'data'" % name) > for (key, value) in members.items(): > check_name(expr_info, "Member of union '%s'" % name, key) > > @@ -613,7 +616,11 @@ def check_alternate(expr, expr_info): > members = expr['data'] > types_seen = {} > > - # Check every branch > + # Check every branch; require at least two branches > + if len(members) < 2: > + raise QAPIExprError(expr_info, > + "Alternate '%s' should have at least two branches " > + "in 'data'" % name) This is stricter than the commit message announced. You can either relax to at least one branch, or amend the commit message. > for (key, value) in members.items(): > check_name(expr_info, "Member of alternate '%s'" % name, key) > > @@ -1059,6 +1066,7 @@ class QAPISchemaObjectTypeVariants(object): > assert bool(tag_member) != bool(tag_name) > assert (isinstance(tag_name, str) or > isinstance(tag_member, QAPISchemaObjectTypeMember)) > + assert len(variants) > 0 > for v in variants: > assert isinstance(v, QAPISchemaObjectTypeVariant) > self.tag_name = tag_name > diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt > index 128f074..999f3b9 100644 > --- a/docs/qapi-code-gen.txt > +++ b/docs/qapi-code-gen.txt > @@ -187,11 +187,11 @@ prevent incomplete include files. > > Usage: { 'struct': STRING, 'data': DICT, '*base': STRUCT-NAME } > > -A struct is a dictionary containing a single 'data' key whose > -value is a dictionary. This corresponds to a struct in C or an Object > -in JSON. Each value of the 'data' dictionary must be the name of a > -type, or a one-element array containing a type name. An example of a > -struct is: > +A struct is a dictionary containing a single 'data' key whose value is > +a dictionary; the dictionary may be empty. This corresponds to a > +struct in C or an Object in JSON. Each value of the 'data' dictionary > +must be the name of a type, or a one-element array containing a type > +name. An example of a struct is: > > { 'struct': 'MyType', > 'data': { 'member1': 'str', 'member2': 'int', '*member3': 'str' } } > @@ -288,9 +288,10 @@ or: { 'union': STRING, 'data': DICT, 'base': STRUCT-NAME, > > Union types are used to let the user choose between several different > variants for an object. There are two flavors: simple (no > -discriminator or base), flat (both discriminator and base). A union > +discriminator or base), and flat (both discriminator and base). A union > type is defined using a data dictionary as explained in the following > -paragraphs. > +paragraphs. The data dictionary for either type of union must not > +be empty. > > A simple union type defines a mapping from automatic discriminator > values to data types like in this example: Missing: update of section "Alternate types". [tests/ diff looks good]
On 02/16/2016 09:08 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Empty unions serve no purpose, and while we compile with gcc >> which permits them, strict C99 forbids them. We could inject >> a dummy member (and in fact, we do for empty structs), but while > > gen_variants() injects void *data. > >> empty structs make sense in qapi, > > Suggest to cut the paragaph until here. Side effect of rebasing - I originally had this patch after one that deletes the 'data' member, but that requires a few other patches. I'll reword it to mention that we want to delete 'data', at which point we would be left with an empty union if we didn't prohibit it at parse time. >> @@ -613,7 +616,11 @@ def check_alternate(expr, expr_info): >> members = expr['data'] >> types_seen = {} >> >> - # Check every branch >> + # Check every branch; require at least two branches >> + if len(members) < 2: >> + raise QAPIExprError(expr_info, >> + "Alternate '%s' should have at least two branches " >> + "in 'data'" % name) > > This is stricter than the commit message announced. You can either > relax to at least one branch, or amend the commit message. Will amend; a later patch actually relies on the 2-or-more promise for alternates, so it is worth documenting as intentional. >> A simple union type defines a mapping from automatic discriminator >> values to data types like in this example: > > Missing: update of section "Alternate types". Already done there (commit 7b1b98c4): "An alternate type is one that allows a choice between two or more JSON data types (string, integer, number, or object, but currently not..." I didn't see anything worth rewording. > > [tests/ diff looks good] >
diff --git a/scripts/qapi.py b/scripts/qapi.py index f40dc9e..f97236f 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -590,7 +590,10 @@ def check_union(expr, expr_info): "Discriminator '%s' must be of enumeration " "type" % discriminator) - # Check every branch + # Check every branch; don't allow an empty union + if len(members) == 0: + raise QAPIExprError(expr_info, + "Union '%s' cannot have empty 'data'" % name) for (key, value) in members.items(): check_name(expr_info, "Member of union '%s'" % name, key) @@ -613,7 +616,11 @@ def check_alternate(expr, expr_info): members = expr['data'] types_seen = {} - # Check every branch + # Check every branch; require at least two branches + if len(members) < 2: + raise QAPIExprError(expr_info, + "Alternate '%s' should have at least two branches " + "in 'data'" % name) for (key, value) in members.items(): check_name(expr_info, "Member of alternate '%s'" % name, key) @@ -1059,6 +1066,7 @@ class QAPISchemaObjectTypeVariants(object): assert bool(tag_member) != bool(tag_name) assert (isinstance(tag_name, str) or isinstance(tag_member, QAPISchemaObjectTypeMember)) + assert len(variants) > 0 for v in variants: assert isinstance(v, QAPISchemaObjectTypeVariant) self.tag_name = tag_name diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt index 128f074..999f3b9 100644 --- a/docs/qapi-code-gen.txt +++ b/docs/qapi-code-gen.txt @@ -187,11 +187,11 @@ prevent incomplete include files. Usage: { 'struct': STRING, 'data': DICT, '*base': STRUCT-NAME } -A struct is a dictionary containing a single 'data' key whose -value is a dictionary. This corresponds to a struct in C or an Object -in JSON. Each value of the 'data' dictionary must be the name of a -type, or a one-element array containing a type name. An example of a -struct is: +A struct is a dictionary containing a single 'data' key whose value is +a dictionary; the dictionary may be empty. This corresponds to a +struct in C or an Object in JSON. Each value of the 'data' dictionary +must be the name of a type, or a one-element array containing a type +name. An example of a struct is: { 'struct': 'MyType', 'data': { 'member1': 'str', 'member2': 'int', '*member3': 'str' } } @@ -288,9 +288,10 @@ or: { 'union': STRING, 'data': DICT, 'base': STRUCT-NAME, Union types are used to let the user choose between several different variants for an object. There are two flavors: simple (no -discriminator or base), flat (both discriminator and base). A union +discriminator or base), and flat (both discriminator and base). A union type is defined using a data dictionary as explained in the following -paragraphs. +paragraphs. The data dictionary for either type of union must not +be empty. A simple union type defines a mapping from automatic discriminator values to data types like in this example: diff --git a/tests/qapi-schema/alternate-empty.err b/tests/qapi-schema/alternate-empty.err index e69de29..bb06c5b 100644 --- a/tests/qapi-schema/alternate-empty.err +++ b/tests/qapi-schema/alternate-empty.err @@ -0,0 +1 @@ +tests/qapi-schema/alternate-empty.json:2: Alternate 'Alt' should have at least two branches in 'data' diff --git a/tests/qapi-schema/alternate-empty.exit b/tests/qapi-schema/alternate-empty.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/alternate-empty.exit +++ b/tests/qapi-schema/alternate-empty.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/alternate-empty.json b/tests/qapi-schema/alternate-empty.json index db3820f..fff15ba 100644 --- a/tests/qapi-schema/alternate-empty.json +++ b/tests/qapi-schema/alternate-empty.json @@ -1,2 +1,2 @@ -# FIXME - alternates should list at least two types to be useful +# alternates must list at least two types to be useful { 'alternate': 'Alt', 'data': { 'i': 'int' } } diff --git a/tests/qapi-schema/alternate-empty.out b/tests/qapi-schema/alternate-empty.out index f78f174..e69de29 100644 --- a/tests/qapi-schema/alternate-empty.out +++ b/tests/qapi-schema/alternate-empty.out @@ -1,5 +0,0 @@ -object :empty -alternate Alt - case i: int -enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool'] - prefix QTYPE diff --git a/tests/qapi-schema/flat-union-empty.err b/tests/qapi-schema/flat-union-empty.err index e69de29..15754f5 100644 --- a/tests/qapi-schema/flat-union-empty.err +++ b/tests/qapi-schema/flat-union-empty.err @@ -0,0 +1 @@ +tests/qapi-schema/flat-union-empty.json:4: Union 'Union' cannot have empty 'data' diff --git a/tests/qapi-schema/flat-union-empty.exit b/tests/qapi-schema/flat-union-empty.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/flat-union-empty.exit +++ b/tests/qapi-schema/flat-union-empty.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/flat-union-empty.json b/tests/qapi-schema/flat-union-empty.json index 67dd297..77f1d9a 100644 --- a/tests/qapi-schema/flat-union-empty.json +++ b/tests/qapi-schema/flat-union-empty.json @@ -1,4 +1,4 @@ -# FIXME - flat unions should not be empty +# flat unions cannot be empty { 'enum': 'Empty', 'data': [ ] } { 'struct': 'Base', 'data': { 'type': 'Empty' } } { 'union': 'Union', 'base': 'Base', 'discriminator': 'type', 'data': { } } diff --git a/tests/qapi-schema/flat-union-empty.out b/tests/qapi-schema/flat-union-empty.out index eade2d5..e69de29 100644 --- a/tests/qapi-schema/flat-union-empty.out +++ b/tests/qapi-schema/flat-union-empty.out @@ -1,9 +0,0 @@ -object :empty -object Base - member type: Empty optional=False -enum Empty [] -enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool'] - prefix QTYPE -object Union - base Base - tag type diff --git a/tests/qapi-schema/union-empty.err b/tests/qapi-schema/union-empty.err index e69de29..12c2022 100644 --- a/tests/qapi-schema/union-empty.err +++ b/tests/qapi-schema/union-empty.err @@ -0,0 +1 @@ +tests/qapi-schema/union-empty.json:2: Union 'Union' cannot have empty 'data' diff --git a/tests/qapi-schema/union-empty.exit b/tests/qapi-schema/union-empty.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/union-empty.exit +++ b/tests/qapi-schema/union-empty.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/union-empty.json b/tests/qapi-schema/union-empty.json index 1785007..1f0b13c 100644 --- a/tests/qapi-schema/union-empty.json +++ b/tests/qapi-schema/union-empty.json @@ -1,2 +1,2 @@ -# FIXME - unions should not be empty +# unions cannot be empty { 'union': 'Union', 'data': { } } diff --git a/tests/qapi-schema/union-empty.out b/tests/qapi-schema/union-empty.out index bdf17e5..e69de29 100644 --- a/tests/qapi-schema/union-empty.out +++ b/tests/qapi-schema/union-empty.out @@ -1,6 +0,0 @@ -object :empty -enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool'] - prefix QTYPE -object Union - member type: UnionKind optional=False -enum UnionKind []
Empty unions serve no purpose, and while we compile with gcc which permits them, strict C99 forbids them. We could inject a dummy member (and in fact, we do for empty structs), but while empty structs make sense in qapi, empty unions don't add any expressiveness to the QMP language. So prohibit them at parse time. Update the documentation and testsuite to match. Signed-off-by: Eric Blake <eblake@redhat.com> --- v10: hoist into earlier series [no v7, v8, or v9] v6: rebase to earlier qapi.py cleanups --- scripts/qapi.py | 12 ++++++++++-- docs/qapi-code-gen.txt | 15 ++++++++------- tests/qapi-schema/alternate-empty.err | 1 + tests/qapi-schema/alternate-empty.exit | 2 +- tests/qapi-schema/alternate-empty.json | 2 +- tests/qapi-schema/alternate-empty.out | 5 ----- tests/qapi-schema/flat-union-empty.err | 1 + tests/qapi-schema/flat-union-empty.exit | 2 +- tests/qapi-schema/flat-union-empty.json | 2 +- tests/qapi-schema/flat-union-empty.out | 9 --------- tests/qapi-schema/union-empty.err | 1 + tests/qapi-schema/union-empty.exit | 2 +- tests/qapi-schema/union-empty.json | 2 +- tests/qapi-schema/union-empty.out | 6 ------ 14 files changed, 27 insertions(+), 35 deletions(-)