[v7,07/14] qapi: Move union tag quirks into subclass

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 <eblake@redhat.com>

---
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(-)

Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
>
> ---
> 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']

On 10/08/2015 06:25 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
>>
>> ---
>> 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.

I had already done the same sort of fix, but it was just sitting later
in my series where you hadn't reached reviewing yet.

> 
> 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.

I also like the subclass idea because it makes simplifying alternates
easier (see my just-posted subset C).

But it sounds like getting rid of the 'type'/'kind' mismatch sooner
rather than later seems like the direction we should be heading.

If I need to spin a v8 of this series, I'll certainly include that
conversion (whether from mine, yours, or a combination of the two).