Re: [Qemu-devel] [PATCH v6 04/12] qapi: Create simple union type member earlier

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> For simple unions, we were creating the implicit 'type' tag
> member during the QAPISchemaObjectTypeVariants constructor.
> This is different from every other implicit QAPISchemaEntity
> object, which get created by QAPISchema methods.  Hoist the
> creation to the caller, and pass the entity rather than the
> string name, so that we have the nice property that no
> entities are created as a side effect within a different
> entity.  A later patch will then have an easier time of
> associating location info with each entity creation.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v6: New patch
> ---
>  scripts/qapi.py | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 15640b6..255001a 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1010,18 +1010,16 @@ class QAPISchemaObjectTypeMember(object):
>
>
>  class QAPISchemaObjectTypeVariants(object):
> -    def __init__(self, tag_name, tag_enum, variants):
> +    def __init__(self, tag_name, tag_member, variants):
>          assert tag_name is None or isinstance(tag_name, str)
> -        assert tag_enum is None or isinstance(tag_enum, str)
> +        assert (tag_member is None or
> +                isinstance(tag_member, QAPISchemaObjectTypeMember))
>          for v in variants:
>              assert isinstance(v, QAPISchemaObjectTypeVariant)
>          self.tag_name = tag_name
>          if tag_name:
> -            assert not tag_enum
> -            self.tag_member = None
> -        else:
> -            self.tag_member = QAPISchemaObjectTypeMember('type', tag_enum,
> -                                                         False)
> +            assert tag_member is None

Since the conditional degenerates to just checking the "either tag_name
of tag_member" precondition, let's move it to right after the checking
of tag_name and tag_member, i.e. before the loop.

I'd also simplify to

        assert not tag_name != not tag_member

or

        bool(tag_name) != bool(tag_member)

> +        self.tag_member = tag_member
>          self.variants = variants

Before:

    __init__() preconditions involving the tag:
        if flat union:
            tag_name is the name of the tag member
            tag_enum is None
        else simple union or alternate:
            tag_name is None
            tag_enum is the name of the implicitly defined enumeration type

    __init__() postconditions:
        if flat union:
            self.tag_name is the name of the tag member
            self.tag_member is None
        else simple union or alternate:
            self.tag_name is None
            self.tag_member is the implicitly defined tag member

    check() postconditions:
        if flat union:
            self.tag_name is the name of the tag member
        else simple union or alternate:
            self.tag_name is None
        self.tag_member is the tag member

After:

    __init__() preconditions involving the tag:
        if flat union:
            tag_name is the name of the tag member
            tag_member is None
        else simple union or alternate:
            tag_name is None
            tag_member is the implicitly defined tag member

    __init__() postconditions:
        exactly as before

    check() postconditions:
        exactly as before

Okay.  Slightly simpler even.

>
>      def check(self, schema, members, seen):
> @@ -1226,8 +1224,9 @@ class QAPISchema(object):
>          return QAPISchemaObjectTypeVariant(case, typ)
>
>      def _make_tag_enum(self, type_name, variants):
> -        return self._make_implicit_enum_type(type_name,
> -                                             [v.name for v in variants])
> +        typ = self._make_implicit_enum_type(type_name,
> +                                            [v.name for v in variants])
> +        return QAPISchemaObjectTypeMember('type', typ, False)

I think this function should now be called _make_tag_member(), or
_make_implicit_tag_member(), or _make_implicit_tag().

>
>      def _def_union_type(self, expr, info):
>          name = expr['union']