From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43809) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOQpx-00081Y-A4 for qemu-devel@nongnu.org; Wed, 27 Jan 2016 09:12:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aOQpt-0006lV-8K for qemu-devel@nongnu.org; Wed, 27 Jan 2016 09:12:45 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47555) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOQps-0006lF-VO for qemu-devel@nongnu.org; Wed, 27 Jan 2016 09:12:41 -0500 From: Markus Armbruster References: <1453219845-30939-1-git-send-email-eblake@redhat.com> <1453219845-30939-32-git-send-email-eblake@redhat.com> Date: Wed, 27 Jan 2016 15:12:38 +0100 In-Reply-To: <1453219845-30939-32-git-send-email-eblake@redhat.com> (Eric Blake's message of "Tue, 19 Jan 2016 09:10:39 -0700") Message-ID: <87egd3jell.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v9 31/37] qapi-visit: Unify struct and union visit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, Michael Roth 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 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 =3D 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, ChardevBac= kend **obj, Error **errp) > | { > | Error *err =3D 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 > Reviewed-by: Marc-Andr=C3=A9 Lureau > > --- > 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 =3D 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 +=3D mcgen(''' > - > -void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **ob= j, Error **errp) > -{ > - Error *err =3D NULL; > - > - visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), &err); > - if (err) { > - goto out; > - } > - if (!*obj) { > - goto out_obj; > - } > -''', > - name=3Dname, c_name=3Dc_name(name)) > - if (base and not base.is_empty()) or members: > - ret +=3D mcgen(''' > - visit_type_%(c_name)s_fields(v, obj, &err); > -''', > - c_name=3Dc_name(name)) > - ret +=3D mcgen(''' > -out_obj: > - error_propagate(errp, err); > - err =3D 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 =3D '' > > assert base > if not base.is_empty(): > ret +=3D gen_visit_fields_decl(base) > + if members: > + ret +=3D 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 =3D 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 +=3D 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 +=3D 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 +=3D mcgen(''' > > void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **ob= j, Error **errp) > @@ -272,61 +238,71 @@ void visit_type_%(c_name)s(Visitor *v, const char *= name, %(c_name)s **obj, Error > ''', > c_name=3Dc_name(name)) > > - if not base.is_empty(): > + if not base.is_empty() or members: > + if members: > + type_name =3D c_name(name) > + cast =3D '' > + else: > + type_name =3D base.c_name() > + cast =3D '(%s **)' % type_name > ret +=3D 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=3Dbase.c_name()) > - else: > + c_name=3Dtype_name, cast=3Dcast) > + if variants: > + ret +=3D gen_err_check(label=3D'out_obj') > + > + if variants: > ret +=3D mcgen(''' > - visit_type_%(c_type)s(v, "%(name)s", &(*obj)->%(c_name)s, &err); > -''', > - c_type=3Dvariants.tag_member.type.c_name(), > - c_name=3Dc_name(variants.tag_member.name), > - name=3Dvariants.tag_member.name) > - ret +=3D gen_err_check(label=3D'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 +=3D mcgen(''' > if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) { > goto out_obj; > } > switch ((*obj)->%(c_name)s) { > ''', > - c_name=3Dc_name(variants.tag_member.name)) > + c_name=3Dc_name(variants.tag_member.name)) > > - for var in variants.variants: > - # TODO ugly special case for simple union > - simple_union_type =3D var.simple_union_type() > - ret +=3D mcgen(''' > + for var in variants.variants: > + # TODO ugly special case for simple union > + simple_union_type =3D var.simple_union_type() > + ret +=3D mcgen(''' > case %(case)s: > ''', > - case=3Dc_enum_const(variants.tag_member.type.name, > - var.name)) > - if simple_union_type: > - ret +=3D mcgen(''' > + case=3Dc_enum_const(variants.tag_member.type.na= me, > + var.name)) > + if simple_union_type: > + ret +=3D mcgen(''' > visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err); > ''', > - c_type=3Dsimple_union_type.c_name(), > - c_name=3Dc_name(var.name)) > - elif not var.type.is_empty(): > - ret +=3D mcgen(''' > + c_type=3Dsimple_union_type.c_name(), > + c_name=3Dc_name(var.name)) > + elif not var.type.is_empty(): > + ret +=3D mcgen(''' > visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err); > ''', > - c_type=3Dvar.type.c_name(), > - c_name=3Dc_name(var.name)) > - ret +=3D mcgen(''' > + c_type=3Dvar.type.c_name(), > + c_name=3Dc_name(var.name)) > + ret +=3D mcgen(''' > break; > ''') > > - ret +=3D mcgen(''' > + ret +=3D mcgen(''' > default: > abort(); > } > +''') > + > + ret +=3D mcgen(''' > out_obj: > +''') > + if variants: > + ret +=3D mcgen(''' > error_propagate(errp, err); > err =3D NULL; > if (*obj) { > visit_end_union(v, !!(*obj)->u.data, &err); > } > +''') > + ret +=3D mcgen(''' > error_propagate(errp, err); > err =3D 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 +=3D gen_visit_decl(name) > - if variants: > - if members: > - # Members other than variants.tag_member not implemented > - assert len(members) =3D=3D 1 > - assert members[0] =3D=3D variants.tag_member > - self.defn +=3D gen_visit_union(name, base, variants) > - else: > - self.defn +=3D gen_visit_struct(name, base, members) > + self.defn +=3D gen_visit_object(name, base, members, variants) > > def visit_alternate_type(self, name, info, variants): > self.decl +=3D gen_visit_decl(name)