| Message ID | 1453219845-30939-32-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <eblake@redhat.com> 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 <eblake@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > 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)
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 -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) + 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) - + # 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) @@ -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') - 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)
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 <eblake@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- 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(-)