[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 31/37] qapi-visit: Unify struct and union vis
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v9 31/37] qapi-visit: Unify struct and union visit |
Date: |
Wed, 27 Jan 2016 15:12:38 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
I'm sending this out even though it's unfinished. It's probably more
useful for documenting my difficulties and confusion than for improving
the code.
Eric Blake <address@hidden> writes:
> We are finally at the point where gen_visit_struct() and
> gen_visit_union() can be unified to a generic gen_visit_object().
>
> The generated code for structs and for flat unions is unchanged.
> For simple unions, a new visit_type_FOO_fields() is created,
> wrapping the visit of the non-variant tag field:
>
> |+static void visit_type_ChardevBackend_fields(Visitor *v, ChardevBackend
> **obj, Error **errp)
> |+{
> |+ Error *err = NULL;
> |+
> |+ visit_type_ChardevBackendKind(v, "type", &(*obj)->type, &err);
> |+ if (err) {
> |+ goto out;
> |+ }
> |+
> |+out:
> |+ error_propagate(errp, err);
> |+}
> |+
> | void visit_type_ChardevBackend(Visitor *v, const char *name, ChardevBackend
> **obj, Error **errp)
> | {
> | Error *err = NULL;
> |@@ -2319,7 +2332,7 @@ void visit_type_ChardevBackend(Visitor *
> | if (!*obj) {
> | goto out_obj;
> | }
> |- visit_type_ChardevBackendKind(v, "type", &(*obj)->type, &err);
> |+ visit_type_ChardevBackend_fields(v, obj, &err);
> | if (err) {
>
> Signed-off-by: Eric Blake <address@hidden>
> Reviewed-by: Marc-André Lureau <address@hidden>
>
> ---
> v9: no change
> v8: rebase to 'name' motion
> v7: rebase to earlier changes
> v6: new patch
> ---
> scripts/qapi-visit.py | 133
> +++++++++++++++++++-------------------------------
> 1 file changed, 51 insertions(+), 82 deletions(-)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 6d5c3d9..feef17f 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -109,46 +109,6 @@ out:
> return ret
>
>
> -def gen_visit_struct(name, base, members):
> - ret = gen_visit_struct_fields(name, base, members)
> -
> - # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
> - # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
> - # rather than leaving it non-NULL. As currently written, the caller must
> - # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
> - ret += mcgen('''
> -
> -void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj,
> Error **errp)
> -{
> - Error *err = NULL;
> -
> - visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), &err);
> - if (err) {
> - goto out;
> - }
> - if (!*obj) {
> - goto out_obj;
> - }
> -''',
> - name=name, c_name=c_name(name))
> - if (base and not base.is_empty()) or members:
> - ret += mcgen('''
> - visit_type_%(c_name)s_fields(v, obj, &err);
> -''',
> - c_name=c_name(name))
> - ret += mcgen('''
> -out_obj:
> - error_propagate(errp, err);
> - err = NULL;
> - visit_end_struct(v, &err);
> -out:
> - error_propagate(errp, err);
> -}
> -''')
> -
> - return ret
> -
> -
> def gen_visit_list(name, element_type):
> # FIXME: if *obj is NULL on entry, and the first visit_next_list()
> # assigns to *obj, while a later one fails, we should clean up *obj
> @@ -244,18 +204,24 @@ out:
> return ret
>
You're merging gen_visit_struct() and gen_visit_union() into
gen_visit_object(). Need to review both the change from
gen_visit_struct() to gen_visit_object(), and from gen_visit_union() to
gen_visit_object(). Diff shows the latter, but not the former. A
manual diff of the old gen_visit_struct() and new gen_visit_union()
doesn't come out helpful.
Recap differences between structs, flat unions and simple unions:
base members variants
struct may be empty may be empty no
flat union non-empty empty yes
simple union empty just the tag yes
>
> -def gen_visit_union(name, base, variants):
> +def gen_visit_object(name, base, members, variants):
> ret = ''
>
> assert base
> if not base.is_empty():
> ret += gen_visit_fields_decl(base)
> + if members:
> + ret += gen_visit_struct_fields(name, base, members)
Flat unions have no members: the new code does nothing.
Simple unions have a single member: we now generate a
visit_type_UNION_fields() for it.
Respective part of gen_visit_struct():
ret = gen_visit_struct_fields(name, base, members)
It looks like you add a gen_visit_fields_decl() when the struct's base
is non-empty. You actually don't, because the additional one in
gen_visit_object() suppresses the one in gen_visit_struct_fields(). I
think.
> + if variants:
> + for var in variants.variants:
> + # Ugly special case for simple union TODO get rid of it
> + if not var.simple_union_type():
> + ret += gen_visit_implicit_struct(var.type)
>
> - for var in variants.variants:
> - # Ugly special case for simple union TODO get rid of it
> - if not var.simple_union_type():
> - ret += gen_visit_implicit_struct(var.type)
> -
Unions always have variants: no change.
Struct's don't: no change.
> + # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
> + # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
> + # rather than leaving it non-NULL. As currently written, the caller must
> + # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
Comment inherited from gen_visit_struct(). Does it apply to unions as
well?
> ret += mcgen('''
>
> void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj,
> Error **errp)
> @@ -272,61 +238,71 @@ void visit_type_%(c_name)s(Visitor *v, const char
> *name, %(c_name)s **obj, Error
> ''',
> c_name=c_name(name))
>
> - if not base.is_empty():
> + if not base.is_empty() or members:
> + if members:
> + type_name = c_name(name)
> + cast = ''
> + else:
> + type_name = base.c_name()
> + cast = '(%s **)' % type_name
> ret += mcgen('''
> - visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err);
> + visit_type_%(c_name)s_fields(v, %(cast)sobj, &err);
> ''',
> - c_name=base.c_name())
> - else:
> + c_name=type_name, cast=cast)
> + if variants:
> + ret += gen_err_check(label='out_obj')
> +
> + if variants:
> ret += mcgen('''
> - visit_type_%(c_type)s(v, "%(name)s", &(*obj)->%(c_name)s, &err);
> -''',
> - c_type=variants.tag_member.type.c_name(),
> - c_name=c_name(variants.tag_member.name),
> - name=variants.tag_member.name)
> - ret += gen_err_check(label='out_obj')
Around here, the diff becomes too hard to read for me.
Please have a look at my "[PATCH 0/3] qapi-visit: Unify struct and union
visit".
> - ret += mcgen('''
> if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
> goto out_obj;
> }
> switch ((*obj)->%(c_name)s) {
> ''',
> - c_name=c_name(variants.tag_member.name))
> + c_name=c_name(variants.tag_member.name))
>
> - for var in variants.variants:
> - # TODO ugly special case for simple union
> - simple_union_type = var.simple_union_type()
> - ret += mcgen('''
> + for var in variants.variants:
> + # TODO ugly special case for simple union
> + simple_union_type = var.simple_union_type()
> + ret += mcgen('''
> case %(case)s:
> ''',
> - case=c_enum_const(variants.tag_member.type.name,
> - var.name))
> - if simple_union_type:
> - ret += mcgen('''
> + case=c_enum_const(variants.tag_member.type.name,
> + var.name))
> + if simple_union_type:
> + ret += mcgen('''
> visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err);
> ''',
> - c_type=simple_union_type.c_name(),
> - c_name=c_name(var.name))
> - elif not var.type.is_empty():
> - ret += mcgen('''
> + c_type=simple_union_type.c_name(),
> + c_name=c_name(var.name))
> + elif not var.type.is_empty():
> + ret += mcgen('''
> visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
> ''',
> - c_type=var.type.c_name(),
> - c_name=c_name(var.name))
> - ret += mcgen('''
> + c_type=var.type.c_name(),
> + c_name=c_name(var.name))
> + ret += mcgen('''
> break;
> ''')
>
> - ret += mcgen('''
> + ret += mcgen('''
> default:
> abort();
> }
> +''')
> +
> + ret += mcgen('''
> out_obj:
> +''')
> + if variants:
> + ret += mcgen('''
> error_propagate(errp, err);
> err = NULL;
> if (*obj) {
> visit_end_union(v, !!(*obj)->u.data, &err);
> }
> +''')
> + ret += mcgen('''
> error_propagate(errp, err);
> err = NULL;
> visit_end_struct(v, &err);
> @@ -387,14 +363,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
>
> def visit_object_type(self, name, info, base, members, variants):
> self.decl += gen_visit_decl(name)
> - if variants:
> - if members:
> - # Members other than variants.tag_member not implemented
> - assert len(members) == 1
> - assert members[0] == variants.tag_member
> - self.defn += gen_visit_union(name, base, variants)
> - else:
> - self.defn += gen_visit_struct(name, base, members)
> + self.defn += gen_visit_object(name, base, members, variants)
>
> def visit_alternate_type(self, name, info, variants):
> self.decl += gen_visit_decl(name)
- [Qemu-devel] [PATCH v9 27/37] qapi: Add type.is_empty() helper, (continued)
- [Qemu-devel] [PATCH v9 27/37] qapi: Add type.is_empty() helper, Eric Blake, 2016/01/19
- [Qemu-devel] [PATCH v9 37/37] qapi: Update docs to match recent generator changes, Eric Blake, 2016/01/19
- [Qemu-devel] [PATCH v9 22/37] qapi: Add visit_type_null() visitor, Eric Blake, 2016/01/19
- [Qemu-devel] [PATCH v9 25/37] spapr_drc: Expose 'null' in qom-get when there is no fdt, Eric Blake, 2016/01/19
- [Qemu-devel] [PATCH v9 24/37] qmp: Tighten output visitor rules, Eric Blake, 2016/01/19
- [Qemu-devel] [PATCH v9 31/37] qapi-visit: Unify struct and union visit, Eric Blake, 2016/01/19
- Re: [Qemu-devel] [PATCH v9 31/37] qapi-visit: Unify struct and union visit,
Markus Armbruster <=
- [Qemu-devel] [PATCH v9 30/37] qapi: Canonicalize missing object to :empty, Eric Blake, 2016/01/19
- [Qemu-devel] [PATCH v9 19/37] qmp: Fix reference-counting of qnull on empty output visit, Eric Blake, 2016/01/19
- [Qemu-devel] [PATCH v9 28/37] qapi: Fix command with named empty argument type, Eric Blake, 2016/01/19
- [Qemu-devel] [PATCH v9 23/37] qmp: Support explicit null during input visit, Eric Blake, 2016/01/19