Re: [Qemu-devel] [PATCH v7 07/14] qapi: Move union tag quirks into subclass

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Right now, simple unions have a quirk of using 'kind' in the C
> struct to match the QMP wire name 'type'.  This has resulted in
> messy clients each doing special cases.  While we plan to
> eventually rename things to match, it is better in the meantime
> to consolidate the quirks into a special subclass, by adding a
> new member.c_name() function.  This will also make it easier
> for reworking how alternate types are laid out in a future
> patch.  Use the new c_name() function where possible.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v7: new patch, but borrows idea of subclass from v6 10/12, as
> well as c_name() from 7/12
> ---
>  scripts/qapi-commands.py |  8 ++++----
>  scripts/qapi-types.py    | 12 +++++-------
>  scripts/qapi-visit.py    | 17 +++++------------
>  scripts/qapi.py          | 15 +++++++++++++--
>  4 files changed, 27 insertions(+), 25 deletions(-)

My immediate reaction to the subclass idea was "instead of encapsulating
the flaw more nicely, why not fix it?"  So gave that a try, see my other
reply.

That said, the diffstat shows the subclass idea doesn't take much code.
May make sense if we feel we shouldn't fix the flaw now.

> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 43a893b..ff52ca9 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -32,8 +32,8 @@ def gen_call(name, arg_type, ret_type):
>      if arg_type:
>          for memb in arg_type.members:
>              if memb.optional:
> -                argstr += 'has_%s, ' % c_name(memb.name)
> -            argstr += '%s, ' % c_name(memb.name)
> +                argstr += 'has_%s, ' % memb.c_name()
> +            argstr += '%s, ' % memb.c_name()
>
>      lhs = ''
>      if ret_type:

Trap for the unwary: most of the time, c_name(FOO) is just fine.  Except
when FOO is T.name, where T is a simple union's implicit tag member.

> @@ -77,11 +77,11 @@ def gen_marshal_vars(arg_type, ret_type):
>                  ret += mcgen('''
>      bool has_%(c_name)s = false;
>  ''',
> -                             c_name=c_name(memb.name))
> +                             c_name=memb.c_name())
>              ret += mcgen('''
>      %(c_type)s %(c_name)s = %(c_null)s;
>  ''',
> -                         c_name=c_name(memb.name),
> +                         c_name=memb.c_name(),
>                           c_type=memb.type.c_type(),
>                           c_null=memb.type.c_null())
>          ret += '\n'
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 227ea5c..34ea318 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -136,9 +136,10 @@ struct %(c_name)s {
>  ''')
>      else:
>          ret += mcgen('''
> -    %(c_type)s kind;
> +    %(c_type)s %(c_name)s;
>  ''',
> -                     c_type=c_name(variants.tag_member.type.name))
> +                     c_type=variants.tag_member.type.c_name(),
> +                     c_name=variants.tag_member.c_name())

My patch does

        ret += gen_struct_field(variants.tag_member.name,
                                variants.tag_member.type,
                                False);

>
>      # 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
> @@ -152,10 +153,7 @@ struct %(c_name)s {
>      union { /* union tag is @%(c_name)s */
>          void *data;
>  ''',
> -                 # TODO ugly special case for simple union
> -                 # Use same tag name in C as on the wire to get rid of
> -                 # it, then: c_name=c_name(variants.tag_member.name)
> -                 c_name=c_name(variants.tag_name or 'kind'))
> +                 c_name=variants.tag_member.c_name())
>
>      for var in variants.variants:
>          # Ugly special case for simple union TODO get rid of it
> @@ -164,7 +162,7 @@ struct %(c_name)s {
>          %(c_type)s %(c_name)s;
>  ''',
>                       c_type=typ.c_type(),
> -                     c_name=c_name(var.name))
> +                     c_name=var.c_name())
>
>      ret += mcgen('''
>      };
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 56b8390..3f74302 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -196,7 +196,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, 
> const char *name, Error
>                       case=c_enum_const(variants.tag_member.type.name,
>                                         var.name),
>                       c_type=var.type.c_name(),
> -                     c_name=c_name(var.name))
> +                     c_name=var.c_name())
>
>      ret += mcgen('''
>      default:
> @@ -249,10 +249,6 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, 
> const char *name, Error
>                       c_name=c_name(name))
>          ret += gen_err_check(label='out_obj')
>
> -    tag_key = variants.tag_member.name
> -    if not variants.tag_name:
> -        # we pointlessly use a different key for simple unions
> -        tag_key = 'type'
>      ret += mcgen('''
>      visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
>      if (err) {
> @@ -264,11 +260,8 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, 
> const char *name, Error
>      switch ((*obj)->%(c_name)s) {
>  ''',
>                   c_type=variants.tag_member.type.c_name(),
> -                 # TODO ugly special case for simple union
> -                 # Use same tag name in C as on the wire to get rid of
> -                 # it, then: c_name=c_name(variants.tag_member.name)
> -                 c_name=c_name(variants.tag_name or 'kind'),
> -                 name=tag_key)
> +                 c_name=variants.tag_member.c_name(),
> +                 name=variants.tag_member.name)
>
>      for var in variants.variants:
>          # TODO ugly special case for simple union
> @@ -283,13 +276,13 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s 
> **obj, const char *name, Error
>          visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "data", &err);
>  ''',
>                           c_type=simple_union_type.c_name(),
> -                         c_name=c_name(var.name))
> +                         c_name=var.c_name())
>          else:
>              ret += mcgen('''
>          visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
>  ''',
>                           c_type=var.type.c_name(),
> -                         c_name=c_name(var.name))
> +                         c_name=var.c_name())
>          ret += mcgen('''
>          break;
>  ''')
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index eaa43b8..f5040da 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -984,7 +984,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>              members = []
>          seen = {}
>          for m in members:
> -            assert c_name(m.name) not in seen
> +            assert m.c_name() not in seen
>              seen[m.name] = m
>          for m in self.local_members:
>              m.check(schema, members, seen)
> @@ -1031,6 +1031,17 @@ class QAPISchemaObjectTypeMember(object):
>          all_members.append(self)
>          seen[self.name] = self
>
> +    def c_name(self):
> +        return c_name(self.name)
> +
> +
> +# TODO Drop this class once we no longer have the 'type'/'kind' mismatch
> +class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
> +    def c_name(self):
> +        assert self.name == 'type'
> +        assert self.type.is_implicit(QAPISchemaEnumType)
> +        return 'kind'
> +
>
>  class QAPISchemaObjectTypeVariants(object):
>      def __init__(self, tag_name, tag_member, variants):
> @@ -1254,7 +1265,7 @@ class QAPISchema(object):
>      def _make_implicit_tag(self, type_name, variants):
>          typ = self._make_implicit_enum_type(type_name,
>                                              [v.name for v in variants])
> -        return QAPISchemaObjectTypeMember('type', typ, False)
> +        return QAPISchemaObjectTypeUnionTag('type', typ, False)
>
>      def _def_union_type(self, expr, info):
>          name = expr['union']