Re: [Qemu-devel] [PATCH v8 06/17] qapi-types: Consolidate gen_struct() and gen_union()

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> These two methods are now close enough that we can finally merge
> them, relying on the fact that simple unions now provide a
> reasonable local_members.  Change gen_struct() to gen_object()
> that handles all forms of QAPISchemaObjectType, and rename and
> shrink gen_union() to gen_variants() to handle the portion of
> gen_object() needed when variants are present.
>
> gen_struct_fields() now has a single caller, so it no longer
> needs an optional parameter; however, I did not choose to inline
> it into the caller.
>
> No difference to generated code.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v8: new patch
> ---
>  scripts/qapi-types.py | 36 +++++++++++-------------------------
>  1 file changed, 11 insertions(+), 25 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index a6bf95d..403768b 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -51,7 +51,7 @@ def gen_struct_field(member):
>      return ret
>
>
> -def gen_struct_fields(local_members, base=None):
> +def gen_struct_fields(local_members, base):
>      ret = ''
>
>      if base:
> @@ -70,7 +70,7 @@ def gen_struct_fields(local_members, base=None):
>      return ret
>
>
> -def gen_struct(name, base, members):
> +def gen_object(name, base, members, variants):
>      ret = mcgen('''
>
>  struct %(c_name)s {
> @@ -79,11 +79,14 @@ struct %(c_name)s {
>
>      ret += gen_struct_fields(members, base)
>
> +    if variants:
> +        ret += gen_variants(variants)
> +
>      # Make sure that all structs have at least one field; this avoids
>      # potential issues with attempting to malloc space for zero-length
>      # structs in C, and also incompatibility with C++ (where an empty
>      # struct is size 1).
> -    if not (base and base.members) and not members:
> +    if not (base and base.members) and not members and not variants:
>          ret += mcgen('''
>      char qapi_dummy_field_for_empty_struct;
>  ''')
> @@ -140,17 +143,7 @@ const int %(c_name)s_qtypes[QTYPE_MAX] = {
>      return ret
>
>
> -def gen_union(name, base, variants):
> -    ret = mcgen('''
> -
> -struct %(c_name)s {
> -''',
> -                c_name=c_name(name))
> -    if base:
> -        ret += gen_struct_fields([], base)
> -    else:
> -        ret += gen_struct_field(variants.tag_member)
> -
> +def gen_variants(variants):
>      # FIXME: What purpose does data serve, besides preventing a union that
>      # has a branch named 'data'? We use it in qapi-visit.py to decide
>      # whether to bypass the switch statement if visiting the discriminator
> @@ -159,11 +152,11 @@ struct %(c_name)s {
>      # should not be any data leaks even without a data pointer.  Or, if
>      # 'data' is merely added to guarantee we don't have an empty union,
>      # shouldn't we enforce that at .json parse time?
> -    ret += mcgen('''
> +    ret = mcgen('''
>      union { /* union tag is @%(c_name)s */
>          void *data;
>  ''',
> -                 c_name=c_name(variants.tag_member.name))
> +                c_name=c_name(variants.tag_member.name))
>
>      for var in variants.variants:
>          # Ugly special case for simple union TODO get rid of it
> @@ -176,7 +169,6 @@ struct %(c_name)s {
>
>      ret += mcgen('''
>      } u;
> -};
>  ''')
>
>      return ret
> @@ -268,13 +260,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>
>      def visit_object_type(self, name, info, base, members, variants):
>          self._fwdecl += gen_fwd_object_or_array(name)
> -        if variants:
> -            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)
> +        self.decl += gen_object(name, base, members, variants)
>          if base:
>              self.decl += gen_upcast(name, base)
>          self._gen_type_cleanup(name)
> @@ -282,7 +268,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>      def visit_alternate_type(self, name, info, variants):
>          self._fwdecl += gen_fwd_object_or_array(name)
>          self._fwdefn += gen_alternate_qtypes(name, variants)
> -        self.decl += gen_union(name, None, variants)
> +        self.decl += gen_object(name, None, [variants.tag_member], variants)
>          self.decl += gen_alternate_qtypes_decl(name)
>          self._gen_type_cleanup(name)

Turned out nicely.

We could morph gen_struct_field() back into its original shape in a
separate patch:

    def gen_struct_fields(members):
        ret = ''

        for memb in members:
            ret += gen_struct_field(memb.name, memb.type, memb.optional)
        return ret

with calling code

    ret += mcgen('''
    /* Members inherited from %(c_name)s: */
''',
                 c_name=base.c_name())
    ret += gen_struct_fields(base.members)
    ret += mcgen('''
    /* Own members: */
''')
    ret += gen_struct_fields(local_members)

Matter of taste.