Message ID | 1445576998-2921-12-git-send-email-eblake@redhat.com |
---|---|
State | New |
Headers | show |
Eric Blake <eblake@redhat.com> writes: > The code for visiting the base class of a child struct created > visit_type_Base_fields() which covers all fields of Base; while > the code for visiting the base class of a flat union created > visit_type_Union_fields() covering all fields of the base > except the discriminator. But since the base class includes > the discriminator of a flat union, we can just visit the entire > base, without needing a separate visit of the discriminator. > Not only is consistently visiting all fields easier to > understand, it lets us share code. > > The generated code in qapi-visit.c loses several now-unused > visit_type_UNION_fields(), along with changes like: > > |@@ -1654,11 +1557,7 @@ void visit_type_BlockdevOptions(Visitor > | if (!*obj) { > | goto out_obj; > | } > |- visit_type_BlockdevOptions_fields(v, obj, &err); > |- if (err) { > |- goto out_obj; > |- } > |- visit_type_BlockdevDriver(v, &(*obj)->driver, "driver", &err); > |+ visit_type_BlockdevOptionsBase_fields(v, (BlockdevOptionsBase **)obj, &err); > | if (err) { > | goto out_obj; > | } > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v10: split out regex to earlier commit, rebase to earlier changes, > improve commit message > v9: (no v6-8): hoist from v5 35/46; rebase to master; fix indentation > botch in gen_visit_union(); polish commit message > --- > scripts/qapi-visit.py | 32 +++++++++++++++----------------- > 1 file changed, 15 insertions(+), 17 deletions(-) > > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index e30062b..3c23c53 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -226,8 +226,7 @@ def gen_visit_union(name, base, variants): > ret = '' > > if base: > - members = [m for m in base.members if m != variants.tag_member] > - ret += gen_visit_struct_fields(name, None, members) > + ret += gen_visit_fields_decl(base) > > for var in variants.variants: > # Ugly special case for simple union TODO get rid of it Instead of generating a _fields() for the union, we'll reuse the base's. May have to generate a forward declaration for it. > @@ -252,31 +251,30 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error > Visit the non-variant members: > if base: A flat union's non-variant members are exactly the base's members, so call the base's _fields(): > ret += mcgen(''' > - visit_type_%(c_name)s_fields(v, obj, &err); > + visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err); > ''', > - c_name=c_name(name)) + c_name=base.c_name()) I stared at the new cast here some until I realized: to call the base's _fields(), we need to cast obj to base. > - ret += gen_err_check(label='out_obj') > - > - tag_key = variants.tag_member.name > - if not variants.tag_name: > - # we pointlessly use a different key for simple unions > - tag_key = 'type' > - ret += mcgen(''' > + c_name=base.c_name()) > + else: A simple union's non-variant member is the implicit tag, and no more, so visit it: > + ret += mcgen(''' > visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err); > - if (err) { > - goto out_obj; > - } > +''', > + c_type=variants.tag_member.type.c_name(), > + # TODO ugly special case for simple union > + # Use same tag name in C as on the wire to get rid of > + # it, then: c_name=c_name(variants.tag_member.name) > + c_name='kind', > + name=variants.tag_member.name) The 'type' vs. 'kind' stupidity never fails to confuse me, and at this time of day I give up, and diff the generated output. It looks okay. Common error check for both branches of the if: > + ret += gen_err_check(label='out_obj') > + ret += mcgen(''' > if (!visit_start_union(v, !!(*obj)->data, &err) || err) { > goto out_obj; > } Visit variant members: > switch ((*obj)->%(c_name)s) { > ''', > - c_type=variants.tag_member.type.c_name(), > # TODO ugly special case for simple union > # Use same tag name in C as on the wire to get rid of > # it, then: c_name=c_name(variants.tag_member.name) > - c_name=c_name(variants.tag_name or 'kind'), > - name=tag_key) > + c_name=c_name(variants.tag_name or 'kind')) > > for var in variants.variants: > # TODO ugly special case for simple union
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index e30062b..3c23c53 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -226,8 +226,7 @@ def gen_visit_union(name, base, variants): ret = '' if base: - members = [m for m in base.members if m != variants.tag_member] - ret += gen_visit_struct_fields(name, None, members) + ret += gen_visit_fields_decl(base) for var in variants.variants: # Ugly special case for simple union TODO get rid of it @@ -252,31 +251,30 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error if base: ret += mcgen(''' - visit_type_%(c_name)s_fields(v, obj, &err); + visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err); ''', - c_name=c_name(name)) - ret += gen_err_check(label='out_obj') - - tag_key = variants.tag_member.name - if not variants.tag_name: - # we pointlessly use a different key for simple unions - tag_key = 'type' - ret += mcgen(''' + c_name=base.c_name()) + else: + ret += mcgen(''' visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err); - if (err) { - goto out_obj; - } +''', + c_type=variants.tag_member.type.c_name(), + # TODO ugly special case for simple union + # Use same tag name in C as on the wire to get rid of + # it, then: c_name=c_name(variants.tag_member.name) + c_name='kind', + name=variants.tag_member.name) + ret += gen_err_check(label='out_obj') + ret += mcgen(''' if (!visit_start_union(v, !!(*obj)->data, &err) || err) { goto out_obj; } switch ((*obj)->%(c_name)s) { ''', - c_type=variants.tag_member.type.c_name(), # TODO ugly special case for simple union # Use same tag name in C as on the wire to get rid of # it, then: c_name=c_name(variants.tag_member.name) - c_name=c_name(variants.tag_name or 'kind'), - name=tag_key) + c_name=c_name(variants.tag_name or 'kind')) for var in variants.variants: # TODO ugly special case for simple union
The code for visiting the base class of a child struct created visit_type_Base_fields() which covers all fields of Base; while the code for visiting the base class of a flat union created visit_type_Union_fields() covering all fields of the base except the discriminator. But since the base class includes the discriminator of a flat union, we can just visit the entire base, without needing a separate visit of the discriminator. Not only is consistently visiting all fields easier to understand, it lets us share code. The generated code in qapi-visit.c loses several now-unused visit_type_UNION_fields(), along with changes like: |@@ -1654,11 +1557,7 @@ void visit_type_BlockdevOptions(Visitor | if (!*obj) { | goto out_obj; | } |- visit_type_BlockdevOptions_fields(v, obj, &err); |- if (err) { |- goto out_obj; |- } |- visit_type_BlockdevDriver(v, &(*obj)->driver, "driver", &err); |+ visit_type_BlockdevOptionsBase_fields(v, (BlockdevOptionsBase **)obj, &err); | if (err) { | goto out_obj; | } Signed-off-by: Eric Blake <eblake@redhat.com> --- v10: split out regex to earlier commit, rebase to earlier changes, improve commit message v9: (no v6-8): hoist from v5 35/46; rebase to master; fix indentation botch in gen_visit_union(); polish commit message --- scripts/qapi-visit.py | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-)