Eric Blake <ebl...@redhat.com> writes:
> Special-casing 'discriminator == {}' for handling anonymous unions
> is getting awkward; since this particular type is not always a
> dictionary on the wire, it is easier to treat it as a completely
> different class of type, "alternate", so that if a type is listed
> in the union_types array, we know it is not an anonymous union.
>
> This patch just further segregates union handling, to make sure that
> anonymous unions are not stored in union_types, and splitting up
> check_union() into separate functions. A future patch will change
> the qapi grammar, and having the segregation already in place will
> make it easier to deal with the distinct meta-type.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
> scripts/qapi-types.py | 6 ++--
> scripts/qapi-visit.py | 4 +--
> scripts/qapi.py | 94
> +++++++++++++++++++++++++++++----------------------
> 3 files changed, 58 insertions(+), 46 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 2390887..c9e0201 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -170,7 +170,7 @@ typedef enum %(name)s
>
> return lookup_decl + enum_decl
>
> -def generate_anon_union_qtypes(expr):
> +def generate_alternate_qtypes(expr):
>
> name = expr['union']
> members = expr['data']
> @@ -181,7 +181,7 @@ const int %(name)s_qtypes[QTYPE_MAX] = {
> name=name)
>
> for key in members:
> - qtype = find_anonymous_member_qtype(members[key])
> + qtype = find_alternate_member_qtype(members[key])
> assert qtype, "Invalid anonymous union member"
>
> ret += mcgen('''
> @@ -408,7 +408,7 @@ for expr in exprs:
> fdef.write(generate_enum_lookup('%sKind' % expr['union'],
> expr['data'].keys()))
> if expr.get('discriminator') == {}:
> - fdef.write(generate_anon_union_qtypes(expr))
> + fdef.write(generate_alternate_qtypes(expr))
> else:
> continue
> fdecl.write(ret)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 3f82bd4..77b0a1f 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -237,7 +237,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s *obj, const
> char *name, Error **er
> ''',
> name=name)
>
> -def generate_visit_anon_union(name, members):
> +def generate_visit_alternate(name, members):
> ret = mcgen('''
>
> void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error
> **errp)
> @@ -302,7 +302,7 @@ def generate_visit_union(expr):
>
> if discriminator == {}:
> assert not base
> - return generate_visit_anon_union(name, members)
> + return generate_visit_alternate(name, members)
>
> enum_define = discriminator_find_enum_define(expr)
> if enum_define:
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 39cc88b..17252e9 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -224,21 +224,16 @@ def find_base_fields(base):
> return None
> return base_struct_define['data']
>
> -# Return the qtype of an anonymous union branch, or None on error.
> -def find_anonymous_member_qtype(qapi_type):
> +# Return the qtype of an alternate branch, or None on error.
> +def find_alternate_member_qtype(qapi_type):
> if builtin_types.has_key(qapi_type):
> return builtin_types[qapi_type]
> elif find_struct(qapi_type):
> return "QTYPE_QDICT"
> elif find_enum(qapi_type):
> return "QTYPE_QSTRING"
> - else:
> - union = find_union(qapi_type)
> - if union:
> - discriminator = union.get('discriminator')
> - if discriminator == {}:
> - return None
> - return "QTYPE_QDICT"
> + elif find_union(qapi_type):
> + return "QTYPE_QDICT"
> return None
>
> # Return the discriminator enum define if discriminator is specified as an
> @@ -276,22 +271,13 @@ def check_union(expr, expr_info):
> discriminator = expr.get('discriminator')
> members = expr['data']
> values = { 'MAX': '(automatic)' }
> - types_seen = {}
>
> - # Three types of unions, determined by discriminator.
> + # Two types of unions, determined by discriminator.
> + assert discriminator != {}
>
> - # If the value of member 'discriminator' is {}, it's an
> - # anonymous union, and must not have a base.
> - if discriminator == {}:
> - enum_define = None
> - if base:
> - raise QAPIExprError(expr_info,
> - "Anonymous union '%s' must not have a base"
> - % name)
> -
> - # Else if the union object has no member 'discriminator', it's an
> + # If the union object has no member 'discriminator', it's an
> # ordinary union. For now, it must not have a base.
> - elif not discriminator:
> + if not discriminator:
> enum_define = None
> if base:
> raise QAPIExprError(expr_info,
> @@ -346,24 +332,46 @@ def check_union(expr, expr_info):
> % (name, key, values[c_key]))
> values[c_key] = key
>
> - # Ensure anonymous unions have no type conflicts.
> - if discriminator == {}:
> - if isinstance(value, list):
> - raise QAPIExprError(expr_info,
> - "Anonymous union '%s' member '%s' must "
> - "not be array type" % (name, key))
> - qtype = find_anonymous_member_qtype(value)
> - if not qtype:
> - raise QAPIExprError(expr_info,
> - "Anonymous union '%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 "
> - "same QObject type as member '%s'"
> - % (name, key, types_seen[qtype]))
> - types_seen[qtype] = key
> +def check_alternate(expr, expr_info):
> + name = expr['union']
> + base = expr.get('base')
> + discriminator = expr.get('discriminator')
> + members = expr['data']
> + values = { 'MAX': '(automatic)' }
> + types_seen = {}
>
> + assert discriminator == {}
> + if base:
> + raise QAPIExprError(expr_info,
> + "Anonymous union '%s' must not have a base"
> + % name)
> +
> + # Check every branch
> + for (key, value) in members.items():
> + # Check for conflicts in the generated enum
> + c_key = _generate_enum_string(key)
> + if c_key in values:
> + raise QAPIExprError(expr_info,
> + "Union '%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 "
> + "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 "
> + "invalid type '%s'" % (name, key, value))
> + if qtype in types_seen:
> + raise QAPIExprError(expr_info,
> + "Anonymous union '%s' member '%s' has "
> + "same QObject type as member '%s'"
> + % (name, key, types_seen[qtype]))
> + types_seen[qtype] = key
>
> def check_enum(expr, expr_info):
> name = expr['enum']
> @@ -393,7 +401,10 @@ def check_exprs(schema):
> if expr.has_key('enum'):
> check_enum(expr, info)
> elif expr.has_key('union'):
> - check_union(expr, info)
> + if expr.get('discriminator') == {}:
> + check_alternate(expr, info)
> + else:
> + check_union(expr, info)
> elif expr.has_key('event'):
> check_event(expr, info)
>
> @@ -535,7 +546,8 @@ def find_struct(name):
>
> def add_union(definition):
> global union_types
> - union_types.append(definition)
> + if definition.get('discriminator') != {}:
> + union_types.append(definition)
>
> def find_union(name):
> global union_types
This is the only unobvious hunk.
union_types is used only through find_union. The hunk makes
find_union(N) return None when N names an anonymous union.
find_union() is used in two places:
* find_alternate_member_qtype()
Patched further up. It really wants only non-anonymous unions, and
this change to find_union() renders its check for anonymous unions
superfluous. Good.
* generate_visit_alternate()
Asserts that each member's type is either a built-in type, a complex
type, a union type, or an enum type.
The change relaxes the assertion not to trigger on anonymous union
types. Why is that okay?
