Eric Blake <ebl...@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 <ebl...@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']
