Re: [Qemu-devel] [PATCH v10 02/13] qapi: Forbid empty unions and useless alternates

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
>
> ---
> 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]