Re: [Qemu-devel] [PATCH v8 05/17] qapi: Track simple union tag in object.local_members

[Markus Armbruster]

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

[...]