Eric Blake <ebl...@redhat.com> writes:
> We were previously creating all unions with an empty list for
> local_members. However, it will make it easier to unify struct
> and union generation if we include the generated tag member in
> local_members. That way, we can have a common code pattern:
> visit the base (if any), visit the local members (if any), visit
> the variants (if any). The local_members of a flat union
> remains empty (because the discriminator is already visited as
> part of the base).
Keeping the implicit tag implicit by not including it in local_members
was a conscious design decision, but if including it makes unifying
struct and union into objects easier, go right ahead.
> The various front end entities then map as follows:
> struct: optional base, optional local_members, no variants
> simple union: no base, one-element local_members, variants with tag_member
> from local_members
> flat union: base, no local_members, variants with tag_member from base
> alternate: no base, no local_members, variants
>
> With the new local members, we require a bit of finesse to
> avoid assertions in the clients. No change to generated code.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
> ---
> v8: new patch
> ---
> scripts/qapi-types.py | 4 +++-
> scripts/qapi-visit.py | 4 +++-
> scripts/qapi.py | 14 ++++++++++----
> tests/qapi-schema/qapi-schema-test.out | 2 ++
> tests/qapi-schema/union-clash-data.out | 1 +
> tests/qapi-schema/union-empty.out | 1 +
> 6 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index b37900f..a6bf95d 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -269,7 +269,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
> def visit_object_type(self, name, info, base, members, variants):
> self._fwdecl += gen_fwd_object_or_array(name)
> if variants:
> - assert not members # not implemented
> + if members:
> + assert len(members) == 1
> + assert members[0] == variants.tag_member
> self.decl += gen_union(name, base, variants)
> else:
> self.decl += gen_struct(name, base, members)
The assertion checks that not passing members to gen_union() won't lose
any. Before the patch, we assert there are none. After the patch, we
assert there's either none or variants.tag_member. Before ans after,
gen_union() takes care of variants.tag_member. Okay.
Let's keep the comment, though. Perhaps like this:
# Members other than variants.tag_member not implemented
assert len(members) == 1
assert members[0] == variants.tag_member
I hope this the whole conditional will eventually be replaced by
self.decl += gen_object(name, base, members, variants)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index f40c3c7..318b8e6 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -360,7 +360,9 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
> def visit_object_type(self, name, info, base, members, variants):
> self.decl += gen_visit_decl(name)
> if variants:
> - assert not members # not implemented
> + if members:
> + assert len(members) == 1
> + assert members[0] == variants.tag_member
> self.defn += gen_visit_union(name, base, variants)
> else:
> self.defn += gen_visit_struct(name, base, members)
Likewise, let's keep the comment.
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 7c50cc4..84ac151 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -957,6 +957,9 @@ class QAPISchemaArrayType(QAPISchemaType):
>
> class QAPISchemaObjectType(QAPISchemaType):
> def __init__(self, name, info, base, local_members, variants):
> + # struct has local_members, optional base, and no variants
> + # flat union has base, variants, and no local_members
> + # simple union has local_members, variants, and no base
> QAPISchemaType.__init__(self, name, info)
> assert base is None or isinstance(base, str)
> for m in local_members:
> @@ -1048,9 +1051,11 @@ class QAPISchemaObjectTypeVariants(object):
> self.variants = variants
>
> def check(self, schema, members, seen):
> - if self.tag_name:
> + if self.tag_name: # flat union
> self.tag_member = seen[self.tag_name]
> - else:
> + elif seen: # simple union
> + assert self.tag_member in seen.itervalues()
> + else: # alternate
> self.tag_member.check(schema, members, seen)
> assert isinstance(self.tag_member.type, QAPISchemaEnumType)
> for v in self.variants:
The test for simple union is hackish.
I guess you want to bypass self.tag_member.check() when for simple
unions, because it's now checked when QAPISchemaObjectType.check() calls
QAPISchemaObjectTypeMember.check() for each of self.local_members.
Could we instead make QAPISchemaAlternateType.check() call
QAPISchemaObjectTypeMember.check() for its implicit tag, and drop the
else here?
> @@ -1270,13 +1275,14 @@ class QAPISchema(object):
> if tag_name:
> variants = [self._make_variant(key, value)
> for (key, value) in data.iteritems()]
> + members = []
> else:
> variants = [self._make_simple_variant(key, value, info)
> for (key, value) in data.iteritems()]
> tag_member = self._make_implicit_tag(name, info, variants)
> + members = [tag_member]
> self._def_entity(
> - QAPISchemaObjectType(name, info, base,
> - self._make_members(OrderedDict(), info),
> + QAPISchemaObjectType(name, info, base, members,
> QAPISchemaObjectTypeVariants(tag_name,
> tag_member,
> variants)))
Two steps squashed together:
1. Simplify _make_members(OrderedDict(), info) to [].
2. Put a simple union's implicit tag_member into the members array
passed to QAPISchemaObjectType().
Okay.
[...]
