[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 12/28] qapi: Introduce 'alternate' to replace
|
From: |
Markus Armbruster |
|
Subject: |
Re: [Qemu-devel] [PATCH v5 12/28] qapi: Introduce 'alternate' to replace anonymous union |
|
Date: |
Thu, 26 Mar 2015 16:42:24 +0100 |
|
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Eric Blake <address@hidden> writes:
> Rather than special-casing "'union':'foo','alternate':{}" as an
> unusual union that can represent a non-dictionary, it is nicer
> to designate a separate meta-type "'alternate':'foo'" for the
> purpose. This involves a lot of documentation tweaks and fallout
> from .json files, but I already split as much as possible into
> earlier commits.
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
> docs/qapi-code-gen.txt | 63
> +++++++++++-----------
> qapi/block-core.json | 6 +--
> scripts/qapi-types.py | 26 ++++++---
> scripts/qapi-visit.py | 17 +++---
> scripts/qapi.py | 41 +++++++-------
> tests/qapi-schema/alternate-array.err | 2 +-
> tests/qapi-schema/alternate-array.json | 3 +-
> tests/qapi-schema/alternate-base.err | 2 +-
> tests/qapi-schema/alternate-base.json | 5 +-
> tests/qapi-schema/alternate-clash.err | 2 +-
> tests/qapi-schema/alternate-clash.json | 5 +-
> tests/qapi-schema/alternate-conflict-dict.err | 2 +-
> tests/qapi-schema/alternate-conflict-dict.json | 5 +-
> tests/qapi-schema/alternate-conflict-string.err | 2 +-
> tests/qapi-schema/alternate-conflict-string.json | 3 +-
> tests/qapi-schema/alternate-good.json | 5 +-
> tests/qapi-schema/alternate-good.out | 4 +-
> tests/qapi-schema/alternate-nested.err | 2 +-
> tests/qapi-schema/alternate-nested.json | 8 ++-
> tests/qapi-schema/alternate-unknown.err | 2 +-
> tests/qapi-schema/alternate-unknown.json | 3 +-
> tests/qapi-schema/flat-union-bad-base.err | 2 +-
> tests/qapi-schema/flat-union-bad-discriminator.err | 2 +-
> .../qapi-schema/flat-union-bad-discriminator.json | 5 +-
> tests/qapi-schema/flat-union-base-union.err | 2 +-
> tests/qapi-schema/flat-union-base-union.json | 2 +-
> tests/qapi-schema/flat-union-no-base.err | 2 +-
> tests/qapi-schema/qapi-schema-test.json | 3 +-
> tests/qapi-schema/qapi-schema-test.out | 2 +-
> tests/qapi-schema/union-invalid-base.err | 2 +-
> 30 files changed, 117 insertions(+), 113 deletions(-)
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index ce9c4b9..8792b94 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -79,16 +79,16 @@ the definition of complex structs that can have mutually
> recursive
> types, and allows for indefinite nesting of QMP that satisfies the
> schema. A type name should not be defined more than once.
>
> -There are six top-level expressions recognized by the parser:
> -'include', 'command', 'type', 'enum', 'union', and 'event'. There are
> -several built-in types, such as 'int' and 'str'; additionally, the
> -top-level expressions can define complex types, enumeration types, and
> -several flavors of union types. The 'command' expression can refer to
> -existing types by name, or list an anonymous type as a dictionary.
> -Listing a type name inside an array refers to a single-dimension array
> -of that type; multi-dimension arrays are not directly supported
> -(although an array of a complex struct that contains an array member
> -is possible).
> +There are seven top-level expressions recognized by the parser:
> +'include', 'command', 'type', 'enum', 'union', 'alternate', and
> +'event'. There are several built-in types, such as 'int' and 'str';
> +additionally, the top-level expressions can define complex types,
> +enumeration types, two flavors of union types, and an alternate type.
> +The 'command' expression can refer to existing types by name, or list
> +an anonymous type as a dictionary. Listing a type name inside an
Preexisting: we can't quite decide whether to use JSON terminology or
our own. 'dictionary' and 'list' are QObject, JSON calls them 'object'
and 'array'. Let's ignore this for now.
> +array refers to a single-dimension array of that type; multi-dimension
> +arrays are not directly supported (although an array of a complex
> +struct that contains an array member is possible).
>
> Types, commands, and events share a common namespace. Therefore,
> generally speaking, type definitions should always use CamelCase for
> @@ -159,8 +159,8 @@ Usage: { 'type': 'str', 'data': 'dict', '*base':
> 'complex-type-name' }
> A complex type 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
> -complex, enum, union, or built-in type, or a one-element array
> -containing a type name. An example of a complex type is:
> +complex, enum, union, alternate, or built-in type, or a one-element
> +array containing a type name. An example of a complex type is:
>
> { 'type': 'MyType',
> 'data': { 'member1': 'str', 'member2': 'int', '*member3': 'str' } }
> @@ -246,14 +246,12 @@ open-coding the field to be type 'str'.
> Usage: { 'union': 'str', 'data': 'dict' }
> or: { 'union': 'str', 'data': 'dict', 'base': 'complex-type-name',
> 'discriminator': 'enum-member-of-base' }
> -or: { 'union': 'str', 'data': 'dict', 'discriminator': {} }
>
> Union types are used to let the user choose between several different
> -data types. There are three flavors: simple (no discriminator), flat
> -(a base type is mandatory, and discriminator is the name of an enum
> -field within that base type), and anonymous (discriminator is an
> -empty dictionary). A union type is defined using a data dictionary as
> -explained in the following paragraphs.
> +data types. There are two flavors: simple (no discriminator or base),
> +and flat (a base type is mandatory, and discriminator is the name of
> +an enum field within that base type). A union type is defined using a
> +data dictionary as explained in the following paragraphs.
Suggest to simplify to
and flat (both discriminator and base). A union type is defined using a
because we'll explain them in more detail further down already.
>
> A simple union type defines a mapping from automatic discriminator
> values to data types like in this example:
> @@ -321,22 +319,23 @@ Resulting in this JSON object:
> "lazy-refcounts": true }
>
>
> -The final flavor of unions is an anonymous union. While the other two
> -union types are always passed as a dictionary in the wire format, an
> -anonymous union instead allows the direct use of different types in
> -its place. As they aren't structured, they don't have any explicit
> -discriminator but use the (QObject) data type of their value as an
> -implicit discriminator. This means that they are restricted to using
> -only one discriminator value per QObject type. For example, you cannot
> -have two different complex types in an anonymous union, or two
> -different integer types.
> +=== Alternate types ===
>
> -Anonymous unions are declared using an empty dictionary as their
> discriminator.
> -The discriminator values never appear on the wire, they are only used in the
> -generated C code. Anonymous unions cannot have a base type.
> +Usage: { 'alternate: 'str', 'data': 'dict' }
>
> - { 'union': 'BlockRef',
> - 'discriminator': {},
> +An alternate type is one that allows a choice between two or more
> +QObject data types (string, integer, number, or dictionary, but not
> +array) on the wire. The definition is similar to a simple union type,
> +where each branch of the dictionary names a type, and where an
> +implicit C enum NameKind is created for the alternate Name. But
> +unlike a union, the discriminator string is never passed on the wire
> +for QMP, instead appearing only in the generated C code. The type on
> +the wire serves an implicit discriminator, which in turn means that an
> +alternate can express a choice between a string and a single complex
> +type (passed as a dictionary), but cannot distinguish between two
> +different complex types. For example:
> +
> + { 'alternate': 'BlockRef',
> 'data': { 'definition': 'BlockdevOptions',
> 'reference': 'str' } }
>
Same as for PATCH 01: should we explain this in terms of JSON types
instead of QObject types?
Perhaps:
An alternate type is one that allows a choice between values of several
distinct types (string, integer, number, or object, but currently not
array) on the wire. The definition is similar to a simple union type,
where each branch of the union names a type. For example:
{ 'alternate': 'BlockRef',
'data': { 'definition': 'BlockdevOptions',
'reference': 'str' } }
Just like for a simple union, an implicit C enum is generated to
enumerate the branches.
Unlike a union, the discriminator string is never passed on the wire for
QMP. The value's JSON type serves an implicit discriminator, which in
turn means that an alternate can only express a choice between types
represented differently in JSON, such as 'string' (JSON string) and
complex type (JSON object). Two different complex types, for instance,
aren't permitted, because both are represented as JSON objects.
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index f525b04..581c448 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1419,8 +1419,7 @@
> #
> # Since: 2.2
> ##
> -{ 'union': 'Qcow2OverlapChecks',
> - 'discriminator': {},
> +{ 'alternate': 'Qcow2OverlapChecks',
> 'data': { 'flags': 'Qcow2OverlapCheckFlags',
> 'mode': 'Qcow2OverlapCheckMode' } }
>
> @@ -1711,8 +1710,7 @@
> #
> # Since: 1.7
> ##
> -{ 'union': 'BlockdevRef',
> - 'discriminator': {},
> +{ 'alternate': 'BlockdevRef',
> 'data': { 'definition': 'BlockdevOptions',
> 'reference': 'str' } }
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index c9e0201..9c8d68c 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -172,7 +172,7 @@ typedef enum %(name)s
>
> def generate_alternate_qtypes(expr):
>
> - name = expr['union']
> + name = expr['alternate']
> members = expr['data']
>
> ret = mcgen('''
> @@ -182,7 +182,7 @@ const int %(name)s_qtypes[QTYPE_MAX] = {
>
> for key in members:
> qtype = find_alternate_member_qtype(members[key])
> - assert qtype, "Invalid anonymous union member"
> + assert qtype, "Invalid alternate member"
>
> ret += mcgen('''
> [ %(qtype)s ] = %(abbrev)s_KIND_%(enum)s,
> @@ -197,9 +197,9 @@ const int %(name)s_qtypes[QTYPE_MAX] = {
> return ret
>
>
> -def generate_union(expr):
> +def generate_union(expr, meta):
>
> - name = expr['union']
> + name = expr[meta]
> typeinfo = expr['data']
>
> base = expr.get('base')
> @@ -243,7 +243,7 @@ struct %(name)s
> ret += mcgen('''
> };
> ''')
> - if discriminator == {}:
> + if meta == 'alternate':
> ret += mcgen('''
> extern const int %(name)s_qtypes[];
> ''',
> @@ -407,8 +407,12 @@ for expr in exprs:
> ret += generate_enum('%sKind' % expr['union'],
> expr['data'].keys())
> fdef.write(generate_enum_lookup('%sKind' % expr['union'],
> expr['data'].keys()))
> - if expr.get('discriminator') == {}:
> - fdef.write(generate_alternate_qtypes(expr))
> + elif expr.has_key('alternate'):
> + ret += generate_fwd_struct(expr['alternate'], expr['data']) + "\n"
> + ret += generate_enum('%sKind' % expr['alternate'],
> expr['data'].keys())
> + fdef.write(generate_enum_lookup('%sKind' % expr['alternate'],
> + expr['data'].keys()))
> + fdef.write(generate_alternate_qtypes(expr))
> else:
> continue
> fdecl.write(ret)
> @@ -438,11 +442,17 @@ for expr in exprs:
> ret += generate_type_cleanup_decl(expr['type'])
> fdef.write(generate_type_cleanup(expr['type']) + "\n")
> elif expr.has_key('union'):
> - ret += generate_union(expr)
> + ret += generate_union(expr, 'union')
> ret += generate_type_cleanup_decl(expr['union'] + "List")
> fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
> ret += generate_type_cleanup_decl(expr['union'])
> fdef.write(generate_type_cleanup(expr['union']) + "\n")
> + elif expr.has_key('alternate'):
> + ret += generate_union(expr, 'alternate')
> + ret += generate_type_cleanup_decl(expr['alternate'] + "List")
> + fdef.write(generate_type_cleanup(expr['alternate'] + "List") + "\n")
> + ret += generate_type_cleanup_decl(expr['alternate'])
> + fdef.write(generate_type_cleanup(expr['alternate']) + "\n")
> elif expr.has_key('enum'):
> ret += generate_type_cleanup_decl(expr['enum'] + "List")
> fdef.write(generate_type_cleanup(expr['enum'] + "List") + "\n")
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 77b0a1f..3e11089 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -256,7 +256,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj,
> const char *name, Error **e
> ''',
> name=name)
>
> - # For anon union, always use the default enum type automatically
> generated
> + # For alternate, always use the default enum type automatically generated
> # as "'%sKind' % (name)"
> disc_type = '%sKind' % (name)
>
> @@ -264,7 +264,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj,
> const char *name, Error **e
> assert (members[key] in builtin_types.keys()
> or find_struct(members[key])
> or find_union(members[key])
> - or find_enum(members[key])), "Invalid anonymous union member"
> + or find_enum(members[key])), "Invalid alternate member"
>
> enum_full_value = generate_enum_full_value(disc_type, key)
> ret += mcgen('''
> @@ -300,10 +300,6 @@ def generate_visit_union(expr):
> base = expr.get('base')
> discriminator = expr.get('discriminator')
>
> - if discriminator == {}:
> - assert not base
> - return generate_visit_alternate(name, members)
> -
> enum_define = discriminator_find_enum_define(expr)
> if enum_define:
> # Use the enum type as discriminator
> @@ -572,6 +568,15 @@ for expr in exprs:
> expr['data'].keys())
> ret += generate_declaration(expr['union'], expr['data'])
> fdecl.write(ret)
> + elif expr.has_key('alternate'):
> + ret = generate_visit_alternate(expr['alternate'], expr['data'])
> + ret += generate_visit_list(expr['alternate'], expr['data'])
> + fdef.write(ret)
> +
> + ret = generate_decl_enum('%sKind' % expr['alternate'],
> + expr['data'].keys())
> + ret += generate_declaration(expr['alternate'], expr['data'])
> + fdecl.write(ret)
> elif expr.has_key('enum'):
> ret = generate_visit_list(expr['enum'], expr['data'])
> ret += generate_visit_enum(expr['enum'], expr['data'])
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 17252e9..018ec45 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -273,11 +273,10 @@ def check_union(expr, expr_info):
> values = { 'MAX': '(automatic)' }
>
> # Two types of unions, determined by discriminator.
> - assert discriminator != {}
>
> # If the union object has no member 'discriminator', it's an
> # ordinary union. For now, it must not have a base.
> - if not discriminator:
> + if discriminator is None:
> enum_define = None
> if base:
> raise QAPIExprError(expr_info,
> @@ -287,18 +286,22 @@ def check_union(expr, expr_info):
> # Else, it's a flat union.
> else:
> # The object must have a member 'base'.
> - if not base:
> + if not isinstance(base, str):
> raise QAPIExprError(expr_info,
> - "Flat union '%s' must have a base field"
> + "Flat union '%s' must have a string base
> field"
> % name)
Sure this bug fix belongs to this patch?
> base_fields = find_base_fields(base)
> if not base_fields:
> raise QAPIExprError(expr_info,
> - "Base '%s' is not a valid type"
> + "Base '%s' is not a valid base type"
> % base)
>
Likewise.
> # The value of member 'discriminator' must name a member of the
> # base type.
> + if not isinstance(discriminator, str):
> + raise QAPIExprError(expr_info,
> + "Flat union '%s' must have a string "
> + "discriminator field" % name)
> discriminator_type = base_fields.get(discriminator)
> if not discriminator_type:
> raise QAPIExprError(expr_info,
Likewise.
The bug fixes are clearly visible in the test results.
> @@ -333,17 +336,15 @@ def check_union(expr, expr_info):
> values[c_key] = key
>
> def check_alternate(expr, expr_info):
> - name = expr['union']
> - base = expr.get('base')
> - discriminator = expr.get('discriminator')
> + name = expr['alternate']
> members = expr['data']
> values = { 'MAX': '(automatic)' }
> types_seen = {}
>
> - assert discriminator == {}
> + base = expr.get('base')
> if base:
> raise QAPIExprError(expr_info,
> - "Anonymous union '%s' must not have a base"
> + "Alternate '%s' must not have a base"
> % name)
>
> # Check every branch
> @@ -352,23 +353,23 @@ def check_alternate(expr, expr_info):
> c_key = _generate_enum_string(key)
> if c_key in values:
> raise QAPIExprError(expr_info,
> - "Union '%s' member '%s' clashes with '%s'"
> + "Alternate '%s' member '%s' clashes with
> '%s'"
> % (name, key, values[c_key]))
> values[c_key] = key
>
> # Ensure alternates have no type conflicts.
> if isinstance(value, list):
> raise QAPIExprError(expr_info,
> - "Anonymous union '%s' member '%s' must "
> + "Alternate '%s' member '%s' must "
> "not be array type" % (name, key))
> qtype = find_alternate_member_qtype(value)
> if not qtype:
> raise QAPIExprError(expr_info,
> - "Anonymous union '%s' member '%s' has "
> + "Alternate '%s' member '%s' has "
> "invalid type '%s'" % (name, key, value))
> if qtype in types_seen:
> raise QAPIExprError(expr_info,
> - "Anonymous union '%s' member '%s' has "
> + "Alternate '%s' member '%s' has "
> "same QObject type as member '%s'"
> % (name, key, types_seen[qtype]))
> types_seen[qtype] = key
> @@ -401,10 +402,9 @@ def check_exprs(schema):
> if expr.has_key('enum'):
> check_enum(expr, info)
> elif expr.has_key('union'):
> - if expr.get('discriminator') == {}:
> - check_alternate(expr, info)
> - else:
> - check_union(expr, info)
> + check_union(expr, info)
> + elif expr.has_key('alternate'):
> + check_alternate(expr, info)
> elif expr.has_key('event'):
> check_event(expr, info)
>
> @@ -436,6 +436,8 @@ def parse_schema(input_file):
> if expr.has_key('union'):
> if not discriminator_find_enum_define(expr):
> add_enum('%sKind' % expr['union'])
> + elif expr.has_key('alternate'):
> + add_enum('%sKind' % expr['alternate'])
>
> # Final pass - validate that exprs make sense
> check_exprs(schema)
> @@ -546,8 +548,7 @@ def find_struct(name):
>
> def add_union(definition):
> global union_types
> - if definition.get('discriminator') != {}:
> - union_types.append(definition)
> + union_types.append(definition)
>
> def find_union(name):
> global union_types
> diff --git a/tests/qapi-schema/alternate-array.err
> b/tests/qapi-schema/alternate-array.err
> index 2638b85..5721ed2 100644
> --- a/tests/qapi-schema/alternate-array.err
> +++ b/tests/qapi-schema/alternate-array.err
> @@ -1 +1 @@
> -tests/qapi-schema/alternate-array.json:4: Anonymous union 'MyUnion' member
> 'two' must not be array type
> +tests/qapi-schema/alternate-array.json:4: Alternate 'Alt' member 'two' must
> not be array type
> diff --git a/tests/qapi-schema/alternate-array.json
> b/tests/qapi-schema/alternate-array.json
> index e330d57..86d8fa0 100644
> --- a/tests/qapi-schema/alternate-array.json
> +++ b/tests/qapi-schema/alternate-array.json
> @@ -1,7 +1,6 @@
> # FIXME: we do not support array branches of anonymous unions yet
> { 'type': 'One',
> 'data': { 'name': 'str' } }
> -{ 'union': 'MyUnion',
> - 'discriminator': {},
> +{ 'alternate': 'Alt',
> 'data': { 'one': 'One',
> 'two': [ 'int' ] } }
> diff --git a/tests/qapi-schema/alternate-base.err
> b/tests/qapi-schema/alternate-base.err
> index a2486b8..4a2566e 100644
> --- a/tests/qapi-schema/alternate-base.err
> +++ b/tests/qapi-schema/alternate-base.err
> @@ -1 +1 @@
> -tests/qapi-schema/alternate-base.json:4: Anonymous union 'MyUnion' must not
> have a base
> +tests/qapi-schema/alternate-base.json:4: Alternate 'Alt' must not have a base
> diff --git a/tests/qapi-schema/alternate-base.json
> b/tests/qapi-schema/alternate-base.json
> index dad7f02..66edc89 100644
> --- a/tests/qapi-schema/alternate-base.json
> +++ b/tests/qapi-schema/alternate-base.json
> @@ -1,7 +1,6 @@
> -# we reject anonymous union with base type
> +# we reject alternate with base type
> { 'type': 'Base',
> 'data': { 'string': 'str' } }
> -{ 'union': 'MyUnion',
> +{ 'alternate': 'Alt',
> 'base': 'Base',
> - 'discriminator': {},
> 'data': { 'number': 'int' } }
> diff --git a/tests/qapi-schema/alternate-clash.err
> b/tests/qapi-schema/alternate-clash.err
> index 1130c12..51bea3e 100644
> --- a/tests/qapi-schema/alternate-clash.err
> +++ b/tests/qapi-schema/alternate-clash.err
> @@ -1 +1 @@
> -tests/qapi-schema/alternate-clash.json:2: Union 'Union1' member 'ONE'
> clashes with 'one'
> +tests/qapi-schema/alternate-clash.json:2: Alternate 'Alt1' member 'ONE'
> clashes with 'one'
> diff --git a/tests/qapi-schema/alternate-clash.json
> b/tests/qapi-schema/alternate-clash.json
> index fa2d27e..3947935 100644
> --- a/tests/qapi-schema/alternate-clash.json
> +++ b/tests/qapi-schema/alternate-clash.json
> @@ -1,4 +1,3 @@
> -# we detect C enum collisions in an anonymous union
> -{ 'union': 'Union1',
> - 'discriminator': {},
> +# we detect C enum collisions in an alternate
> +{ 'alternate': 'Alt1',
> 'data': { 'one': 'str', 'ONE': 'int' } }
> diff --git a/tests/qapi-schema/alternate-conflict-dict.err
> b/tests/qapi-schema/alternate-conflict-dict.err
> index f256d51..c535332 100644
> --- a/tests/qapi-schema/alternate-conflict-dict.err
> +++ b/tests/qapi-schema/alternate-conflict-dict.err
> @@ -1 +1 @@
> -tests/qapi-schema/alternate-conflict-dict.json:6: Anonymous union 'MyUnion'
> member 'two' has same QObject type as member 'one'
> +tests/qapi-schema/alternate-conflict-dict.json:6: Alternate 'Alt' member
> 'two' has same QObject type as member 'one'
> diff --git a/tests/qapi-schema/alternate-conflict-dict.json
> b/tests/qapi-schema/alternate-conflict-dict.json
> index 177c163..eaac5b3 100644
> --- a/tests/qapi-schema/alternate-conflict-dict.json
> +++ b/tests/qapi-schema/alternate-conflict-dict.json
> @@ -1,9 +1,8 @@
> -# we reject anonymous unions with multiple object branches
> +# we reject alternates with multiple object branches
> { 'type': 'One',
> 'data': { 'name': 'str' } }
> { 'type': 'Two',
> 'data': { 'value': 'int' } }
> -{ 'union': 'MyUnion',
> - 'discriminator': {},
> +{ 'alternate': 'Alt',
> 'data': { 'one': 'One',
> 'two': 'Two' } }
> diff --git a/tests/qapi-schema/alternate-conflict-string.err
> b/tests/qapi-schema/alternate-conflict-string.err
> index a3b7b6d..18ef0d3 100644
> --- a/tests/qapi-schema/alternate-conflict-string.err
> +++ b/tests/qapi-schema/alternate-conflict-string.err
> @@ -1 +1 @@
> -tests/qapi-schema/alternate-conflict-string.json:4: Anonymous union
> 'MyUnion' member 'two' has same QObject type as member 'one'
> +tests/qapi-schema/alternate-conflict-string.json:4: Alternate 'Alt' member
> 'two' has same QObject type as member 'one'
> diff --git a/tests/qapi-schema/alternate-conflict-string.json
> b/tests/qapi-schema/alternate-conflict-string.json
> index a1b0bea..ce00307 100644
> --- a/tests/qapi-schema/alternate-conflict-string.json
> +++ b/tests/qapi-schema/alternate-conflict-string.json
> @@ -1,8 +1,7 @@
> # we reject anonymous unions with multiple string-like branches
> { 'enum': 'Enum',
> 'data': [ 'hello', 'world' ] }
> -{ 'union': 'MyUnion',
> - 'discriminator': {},
> +{ 'alternate': 'Alt',
> 'data': { 'one': 'str',
> 'two': 'Enum' } }
>
> diff --git a/tests/qapi-schema/alternate-good.json
> b/tests/qapi-schema/alternate-good.json
> index 1068e2f..74f338c 100644
> --- a/tests/qapi-schema/alternate-good.json
> +++ b/tests/qapi-schema/alternate-good.json
> @@ -1,10 +1,9 @@
> -# Working example of anonymous union
> +# Working example of alternate
> { 'type': 'Data',
> 'data': { '*number': 'int', '*name': 'str' } }
> { 'enum': 'Enum',
> 'data': [ 'hello', 'world' ] }
> -{ 'union': 'MyUnion',
> - 'discriminator': {},
> +{ 'alternate': 'Alt',
> 'data': { 'value': 'int',
> 'string': 'Enum',
> 'struct': 'Data' } }
> diff --git a/tests/qapi-schema/alternate-good.out
> b/tests/qapi-schema/alternate-good.out
> index b5117d1..c3a6b77 100644
> --- a/tests/qapi-schema/alternate-good.out
> +++ b/tests/qapi-schema/alternate-good.out
> @@ -1,6 +1,6 @@
> [OrderedDict([('type', 'Data'), ('data', OrderedDict([('*number', 'int'),
> ('*name', 'str')]))]),
> OrderedDict([('enum', 'Enum'), ('data', ['hello', 'world'])]),
> - OrderedDict([('union', 'MyUnion'), ('discriminator', OrderedDict()),
> ('data', OrderedDict([('value', 'int'), ('string', 'Enum'), ('struct',
> 'Data')]))])]
> + OrderedDict([('alternate', 'Alt'), ('data', OrderedDict([('value', 'int'),
> ('string', 'Enum'), ('struct', 'Data')]))])]
> [{'enum_name': 'Enum', 'enum_values': ['hello', 'world']},
> - {'enum_name': 'MyUnionKind', 'enum_values': None}]
> + {'enum_name': 'AltKind', 'enum_values': None}]
> [OrderedDict([('type', 'Data'), ('data', OrderedDict([('*number', 'int'),
> ('*name', 'str')]))])]
> diff --git a/tests/qapi-schema/alternate-nested.err
> b/tests/qapi-schema/alternate-nested.err
> index 59df96e..00b05c6 100644
> --- a/tests/qapi-schema/alternate-nested.err
> +++ b/tests/qapi-schema/alternate-nested.err
> @@ -1 +1 @@
> -tests/qapi-schema/alternate-nested.json:5: Anonymous union 'Union2' member
> 'nested' has invalid type 'Union1'
> +tests/qapi-schema/alternate-nested.json:4: Alternate 'Alt2' member 'nested'
> has invalid type 'Alt1'
> diff --git a/tests/qapi-schema/alternate-nested.json
> b/tests/qapi-schema/alternate-nested.json
> index ed2b6b7..2a115b9 100644
> --- a/tests/qapi-schema/alternate-nested.json
> +++ b/tests/qapi-schema/alternate-nested.json
> @@ -1,7 +1,5 @@
> # we reject a nested anonymous union branch
> -{ 'union': 'Union1',
> - 'discriminator': {},
> +{ 'alternate': 'Alt1',
> 'data': { 'name': 'str', 'value': 'int' } }
> -{ 'union': 'Union2',
> - 'discriminator': {},
> - 'data': { 'nested': 'Union1' } }
> +{ 'alternate': 'Alt2',
> + 'data': { 'nested': 'Alt1' } }
> diff --git a/tests/qapi-schema/alternate-unknown.err
> b/tests/qapi-schema/alternate-unknown.err
> index bf8e9ae..7af1b4c 100644
> --- a/tests/qapi-schema/alternate-unknown.err
> +++ b/tests/qapi-schema/alternate-unknown.err
> @@ -1 +1 @@
> -tests/qapi-schema/alternate-unknown.json:2: Anonymous union 'Union' member
> 'unknown' has invalid type 'MissingType'
> +tests/qapi-schema/alternate-unknown.json:2: Alternate 'Alt' member 'unknown'
> has invalid type 'MissingType'
> diff --git a/tests/qapi-schema/alternate-unknown.json
> b/tests/qapi-schema/alternate-unknown.json
> index 0c305c2..d7b217e 100644
> --- a/tests/qapi-schema/alternate-unknown.json
> +++ b/tests/qapi-schema/alternate-unknown.json
> @@ -1,4 +1,3 @@
> # we reject an anonymous union with unknown type in branch
> -{ 'union': 'Union',
> - 'discriminator': {},
> +{ 'alternate': 'Alt',
> 'data': { 'unknown': 'MissingType' } }
> diff --git a/tests/qapi-schema/flat-union-bad-base.err
> b/tests/qapi-schema/flat-union-bad-base.err
> index 5962ff4..f9c31b2 100644
> --- a/tests/qapi-schema/flat-union-bad-base.err
> +++ b/tests/qapi-schema/flat-union-bad-base.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-bad-base.json:9: Base 'OrderedDict([('enum1',
> 'TestEnum'), ('kind', 'str')])' is not a valid type
> +tests/qapi-schema/flat-union-bad-base.json:9: Flat union 'TestUnion' must
> have a string base field
Bug fix visible here...
> diff --git a/tests/qapi-schema/flat-union-bad-discriminator.err
> b/tests/qapi-schema/flat-union-bad-discriminator.err
> index 628432f..4f0c798 100644
> --- a/tests/qapi-schema/flat-union-bad-discriminator.err
> +++ b/tests/qapi-schema/flat-union-bad-discriminator.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-bad-discriminator.json:10: Simple union
> 'TestUnion' must not have a base
> +tests/qapi-schema/flat-union-bad-discriminator.json:11: Flat union
> 'TestUnion' must have a string discriminator field
... and here ...
> diff --git a/tests/qapi-schema/flat-union-bad-discriminator.json
> b/tests/qapi-schema/flat-union-bad-discriminator.json
> index 1599a59..982f072 100644
> --- a/tests/qapi-schema/flat-union-bad-discriminator.json
> +++ b/tests/qapi-schema/flat-union-bad-discriminator.json
> @@ -1,4 +1,5 @@
> -# FIXME: we should require the discriminator to be a string naming a
> base-type member
> +# we require the discriminator to be a string naming a base-type member
> +# this tests the old syntax for anonymous unions before we added alternates
> { 'enum': 'TestEnum',
> 'data': [ 'value1', 'value2' ] }
> { 'type': 'TestBase',
> @@ -9,6 +10,6 @@
> 'data': { 'integer': 'int' } }
> { 'union': 'TestUnion',
> 'base': 'TestBase',
> - 'discriminator': [],
> + 'discriminator': {},
> 'data': { 'kind1': 'TestTypeA',
> 'kind2': 'TestTypeB' } }
> diff --git a/tests/qapi-schema/flat-union-base-union.err
> b/tests/qapi-schema/flat-union-base-union.err
> index 185bf51..318c047 100644
> --- a/tests/qapi-schema/flat-union-base-union.err
> +++ b/tests/qapi-schema/flat-union-base-union.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-base-union.json:11: Base 'UnionBase' is not a
> valid type
> +tests/qapi-schema/flat-union-base-union.json:11: Base 'UnionBase' is not a
> valid base type
> diff --git a/tests/qapi-schema/flat-union-base-union.json
> b/tests/qapi-schema/flat-union-base-union.json
> index bbaa2da..135332d 100644
> --- a/tests/qapi-schema/flat-union-base-union.json
> +++ b/tests/qapi-schema/flat-union-base-union.json
> @@ -1,4 +1,4 @@
> -# FIXME: the error message needs help: we require the base to be a struct
> +# we require the base to be a struct
> { 'enum': 'TestEnum',
> 'data': [ 'value1', 'value2' ] }
> { 'type': 'TestTypeA',
> diff --git a/tests/qapi-schema/flat-union-no-base.err
> b/tests/qapi-schema/flat-union-no-base.err
> index eaf3592..338bff1 100644
> --- a/tests/qapi-schema/flat-union-no-base.err
> +++ b/tests/qapi-schema/flat-union-no-base.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-no-base.json:8: Flat union 'TestUnion' must
> have a base field
> +tests/qapi-schema/flat-union-no-base.json:8: Flat union 'TestUnion' must
> have a string base field
... and here.
> diff --git a/tests/qapi-schema/qapi-schema-test.json
> b/tests/qapi-schema/qapi-schema-test.json
> index e1d35e1..dec8a7c 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -53,8 +53,7 @@
> 'discriminator': 'enum1',
> 'data': { 'value1' : 'UserDefC', 'value2' : 'UserDefB', 'value3' :
> 'UserDefA' } }
>
> -{ 'union': 'UserDefAlternate',
> - 'discriminator': {},
> +{ 'alternate': 'UserDefAlternate',
> 'data': { 'uda': 'UserDefA', 's': 'str', 'i': 'int' } }
>
> # for testing native lists
> diff --git a/tests/qapi-schema/qapi-schema-test.out
> b/tests/qapi-schema/qapi-schema-test.out
> index b55ab8d..313ecf3 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -10,7 +10,7 @@
> OrderedDict([('type', 'UserDefUnionBase'), ('data', OrderedDict([('string',
> 'str'), ('enum1', 'EnumOne')]))]),
> OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefUnionBase'),
> ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefA'),
> ('value2', 'UserDefB'), ('value3', 'UserDefB')]))]),
> OrderedDict([('union', 'UserDefFlatUnion2'), ('base', 'UserDefUnionBase'),
> ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefC'),
> ('value2', 'UserDefB'), ('value3', 'UserDefA')]))]),
> - OrderedDict([('union', 'UserDefAlternate'), ('discriminator',
> OrderedDict()), ('data', OrderedDict([('uda', 'UserDefA'), ('s', 'str'),
> ('i', 'int')]))]),
> + OrderedDict([('alternate', 'UserDefAlternate'), ('data',
> OrderedDict([('uda', 'UserDefA'), ('s', 'str'), ('i', 'int')]))]),
> OrderedDict([('union', 'UserDefNativeListUnion'), ('data',
> OrderedDict([('integer', ['int']), ('s8', ['int8']), ('s16', ['int16']),
> ('s32', ['int32']), ('s64', ['int64']), ('u8', ['uint8']), ('u16',
> ['uint16']), ('u32', ['uint32']), ('u64', ['uint64']), ('number',
> ['number']), ('boolean', ['bool']), ('string', ['str']), ('sizes',
> ['size'])]))]),
> OrderedDict([('command', 'user_def_cmd'), ('data', OrderedDict())]),
> OrderedDict([('command', 'user_def_cmd1'), ('data', OrderedDict([('ud1a',
> 'UserDefOne')]))]),
> diff --git a/tests/qapi-schema/union-invalid-base.err
> b/tests/qapi-schema/union-invalid-base.err
> index 3cc82c0..bedec06 100644
> --- a/tests/qapi-schema/union-invalid-base.err
> +++ b/tests/qapi-schema/union-invalid-base.err
> @@ -1 +1 @@
> -tests/qapi-schema/union-invalid-base.json:8: Base 'int' is not a valid type
> +tests/qapi-schema/union-invalid-base.json:8: Base 'int' is not a valid base
> type
A few 'anonymous' linger in tests/qapi-schema/:
tests/qapi-schema/alternate-array.json:# FIXME: we do not support array
branches of anonymous unions yet
tests/qapi-schema/alternate-conflict-string.json:# we reject anonymous unions
with multiple string-like branches
tests/qapi-schema/alternate-nested.json:# we reject a nested anonymous union
branch
tests/qapi-schema/alternate-unknown.json:# we reject an anonymous union with
unknown type in branch
tests/qapi-schema/flat-union-bad-base.json:# FIXME: should we allow an
anonymous inline base type?
tests/qapi-schema/flat-union-bad-discriminator.json:# this tests the old syntax
for anonymous unions before we added alternates
Since I found nothing wrong with this patch, just a few opportunities
for improvement, and bug fixes that should perhaps be separate:
Reviewed-by: Markus Armbruster <address@hidden>
- Re: [Qemu-devel] [PATCH v5 08/28] qapi: Better error messages for bad unions, (continued)
- [Qemu-devel] [PATCH v5 04/28] qapi: Add some enum tests, Eric Blake, 2015/03/24
- [Qemu-devel] [PATCH v5 07/28] qapi: Simplify tests of simple unions, Eric Blake, 2015/03/24
- [Qemu-devel] [PATCH v5 09/28] qapi: Prepare for catching more semantic parse errors, Eric Blake, 2015/03/24
- [Qemu-devel] [PATCH v5 12/28] qapi: Introduce 'alternate' to replace anonymous union, Eric Blake, 2015/03/24
- [Qemu-devel] [PATCH v5 13/28] qapi: Add some expr tests, Eric Blake, 2015/03/24
[Qemu-devel] [PATCH v5 18/28] qapi: Unify type bypass and add tests, Eric Blake, 2015/03/24