Eric Blake <ebl...@redhat.com> writes: > We initially created the static visit_type_FOO_fields() helper > function for reuse of code - we have cases where the initial > setup for a visit has different allocation (depending on whether > the fields represent a stand-alone type or are embedded as part > of a larger type), but where the actual field visits are > identical once a pointer is available. > > Up until the previous patch, visit_type_FOO_fields() was only > used for structs (no variants), so it was covering every field > for each type where it was emitted. > > Meanwhile, the code for visiting unions looks like: > > static visit_type_U_fields() { > visit base; > visit local_members; > } > visit_type_U() { > visit_start_struct(); > visit_type_U_fields(); > visit variants; > visit_end_struct(); > } > > which splits the fields of the union visit across two functions. > Move the code to visit variants to live inside visit_type_U_fields(), > while making it conditional on having variants so that all other > instances of the helper function remain unchanged. This is also > a step closer towards unifying struct and union visits, and towards > allowing one union type to be the branch of another flat union. > > The resulting diff to the generated code is a bit hard to read,
Mostly because a few visit_type_T_fields() get generated in a different place. If I move them back manually for a diff, the patch's effect becomes clear enough: the switch to visit the variants and the visit_start_union() guarding it moves from visit_type_T() to visit_type_T_fields(). That's what the commit message promises. > but it can be verified that it touches only union types, and that > the end result is the following general structure: > > static visit_type_U_fields() { > visit base; > visit local_members; > visit variants; > } > visit_type_U() { > visit_start_struct(); > visit_type_U_fields(); > visit_end_struct(); > } > > Signed-off-by: Eric Blake <ebl...@redhat.com> > > --- > v11: new patch > --- > scripts/qapi-visit.py | 104 > +++++++++++++++++++++++++------------------------- > 1 file changed, 53 insertions(+), 51 deletions(-) > > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index 1051710..6cea7d6 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -71,11 +71,16 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, > %(c_type)s **obj, Error * > return ret > > > -def gen_visit_struct_fields(name, base, members): > +def gen_visit_struct_fields(name, base, members, variants=None): Two callers: * gen_visit_union(): the new code here comes from there, except it's now completely guarded by if variants. Effect: generated code motion. * gen_visit_struct(): passes no variants, guard false, thus no change. I think the patch becomes slightly clearer if you make the variants parameter mandatory. It'll probably become mandatory anyway when we unify gen_visit_struct() and gen_visit_union(). > ret = '' > > if base: > ret += gen_visit_fields_decl(base) > + 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) > > struct_fields_seen.add(name) > ret += mcgen(''' > @@ -96,8 +101,49 @@ static void visit_type_%(c_name)s_fields(Visitor *v, > %(c_name)s **obj, Error **e > > ret += gen_visit_fields(members, prefix='(*obj)->') > > - # 'goto out' produced for base, and by gen_visit_fields() for each member > - if base or members: > + if variants: > + ret += mcgen(''' > + if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) { > + goto out; > + } > + switch ((*obj)->%(c_name)s) { > +''', > + 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(''' > + case %(case)s: > +''', > + case=c_enum_const(variants.tag_member.type.name, > + var.name, > + variants.tag_member.type.prefix)) > + 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)) > + else: > + 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(''' > + break; > +''') > + > + ret += mcgen(''' > + default: > + abort(); > + } > +''') > + > + # 'goto out' produced for base, by gen_visit_fields() for each member, > + # and if variants were present > + if base or members or variants: > ret += mcgen(''' > > out: > @@ -239,12 +285,7 @@ out: > > > def gen_visit_union(name, base, members, variants): > - ret = gen_visit_struct_fields(name, base, members) > - > - 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) > + ret = gen_visit_struct_fields(name, base, members, variants) > > ret += mcgen(''' > > @@ -260,54 +301,15 @@ void visit_type_%(c_name)s(Visitor *v, const char > *name, %(c_name)s **obj, Error > goto out_obj; > } > visit_type_%(c_name)s_fields(v, obj, &err); > -''', > - c_name=c_name(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)) > - > - 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, > - variants.tag_member.type.prefix)) > - 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)) > - else: > - 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(''' > - break; > -''') > - > - ret += mcgen(''' > - default: > - abort(); > - } > -out_obj: > error_propagate(errp, err); > err = NULL; > +out_obj: > visit_end_struct(v, &err); > out: > error_propagate(errp, err); > } > -''') > +''', > + c_name=c_name(name)) > > return ret