| Message ID | 1455778109-6278-7-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@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 <eblake@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
On 02/18/2016 06:58 AM, Markus Armbruster wrote: > Eric Blake <eblake@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: Maybe reword this to: Meanwhile, the previous patch updated the code for visiting unions to look like: >> 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. I debated about splitting out a separate patch so that the motion of visit_type_T_fields() is separate from the variant visiting, but it wasn't worth the effort. I'm glad you managed to see through the churn in the generated code. >> -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(). You beat me to it - yes, I realized that after sending the series. We can either squash in the fix here to make variants mandatory (and gen_visit_struct() pass an explicit None, which disappears in the next patch), or squash in a fix to 7/15 to delete the '=None'. Either way, I'm fine if you tackle that, if no other major findings turn up.
Eric Blake <eblake@redhat.com> writes: > On 02/18/2016 06:58 AM, Markus Armbruster wrote: >> Eric Blake <eblake@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: > > Maybe reword this to: > > Meanwhile, the previous patch updated the code for visiting unions to > look like: I don't mind. >>> 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. > > I debated about splitting out a separate patch so that the motion of > visit_type_T_fields() is separate from the variant visiting, but it > wasn't worth the effort. I'm glad you managed to see through the churn > in the generated code. > > >>> -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(). > > You beat me to it - yes, I realized that after sending the series. We > can either squash in the fix here to make variants mandatory (and > gen_visit_struct() pass an explicit None, which disappears in the next > patch), or squash in a fix to 7/15 to delete the '=None'. Either way, > I'm fine if you tackle that, if no other major findings turn up. Can do.
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): 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
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, 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 <eblake@redhat.com> --- v11: new patch --- scripts/qapi-visit.py | 104 +++++++++++++++++++++++++------------------------- 1 file changed, 53 insertions(+), 51 deletions(-)