From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37611) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoxJp-0007z9-Tx for qemu-devel@nongnu.org; Wed, 21 Oct 2015 13:36:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZoxJm-0000PP-MC for qemu-devel@nongnu.org; Wed, 21 Oct 2015 13:36:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58653) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoxJm-0000PC-CP for qemu-devel@nongnu.org; Wed, 21 Oct 2015 13:36:54 -0400 From: Markus Armbruster References: <1444968943-11254-1-git-send-email-eblake@redhat.com> <1444968943-11254-7-git-send-email-eblake@redhat.com> Date: Wed, 21 Oct 2015 19:36:51 +0200 In-Reply-To: <1444968943-11254-7-git-send-email-eblake@redhat.com> (Eric Blake's message of "Thu, 15 Oct 2015 22:15:31 -0600") Message-ID: <87oafsdsy4.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v9 06/17] qapi-visit: Remove redundant functions for flat union base List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Michael Roth Eric Blake 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 if the base class were to always > visit all its fields, then we wouldn't need a separate visit of > the discriminator for a union. Not only is consistently > visiting all fields easier to understand, it lets us share code. > > Now that gen_visit_struct_fields() can potentially collect more > than one function into 'ret', a regular expression searching for > whether a label was used may hit a false positive within the > body of the first function. But using a regex was overkill, > since we can easily determine when we jumped to a label. > > Signed-off-by: Eric Blake > > --- > 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 | 35 +++++++++++++++++------------------ > 1 file changed, 17 insertions(+), 18 deletions(-) > > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index 8aae8da..91bf350 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -90,7 +90,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e > > ret += gen_visit_fields(members, prefix='(*obj)->') > > - if re.search('^ *goto out;', ret, re.MULTILINE): > + if base or members: What if we have an empty base and no members? Empty base is a pathological case, admittedly. > ret += mcgen(''' > > out: > @@ -221,8 +221,8 @@ 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_struct_fields(base.name, base.base, > + base.local_members) > > for var in variants.variants: > # Ugly special case for simple union TODO get rid of it > @@ -247,31 +247,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 Diff is confusing (not your fault). Let me compare code before and after real slow. = Before = def gen_visit_union(name, base, variants): ret = '' 0. base is None if and only if the union is simple. 1. If it's a flat union, generate its visit_type_NAME_fields(). This function visits the union's non-variant members *except* the discriminator. Since a simple union has no non-variant members other than the discriminator, generate it only for flat unions. if base: members = [m for m in base.members if m != variants.tag_member] ret += gen_visit_struct_fields(name, None, members) 2. Generate the visit_type_implicit_FOO() we're going to need. 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) 3. Generate its visit_type_NAME(). ret += mcgen(''' void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp) { Error *err = NULL; visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err); if (err) { goto out; } if (!*obj) { goto out_obj; } ''', c_name=c_name(name), name=name) 3.a. If it's a flat union, generate the call of visit_type_NAME_fields(). Not necessary for simple unions, see 1. if base: ret += mcgen(''' visit_type_%(c_name)s_fields(v, obj, &err); ''', c_name=c_name(name)) ret += gen_err_check(label='out_obj') 3.b. Generate visit of discriminator. 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(''' visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err); if (err) { goto out_obj; } 3.c. Generate visit of the active variant. 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) [Some stuff the patch doesn't change omitted...] out_obj: error_propagate(errp, err); err = NULL; if (*obj) { visit_end_union(v, !!(*obj)->data, &err); } error_propagate(errp, err); err = NULL; visit_end_struct(v, &err); out: error_propagate(errp, err); } ''') return ret = After = def gen_visit_union(name, base, variants): ret = '' 0. base is None if and only if the union is simple. 1. If it's a flat union, generate its visit_type_NAME_fields(). This function visits the union's non-variant members *including* the discriminator. However, we generate it only for flat unions. Simple unions have no non-variant members other than the discriminator. if base: ret += gen_visit_struct_fields(base.name, base.base, base.local_members) 2. Generate the visit_type_implicit_FOO() we're going to need. 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) 3. Generate its visit_type_NAME(). ret += mcgen(''' void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp) { Error *err = NULL; visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err); if (err) { goto out; } if (!*obj) { goto out_obj; } ''', c_name=c_name(name), name=name) 3.a. If it's a flat union, generate the call of visit_type_NAME_fields(). Not done for simple unions, see 1. if base: ret += mcgen(''' visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err); ''', c_name=base.c_name()) 3.b. If it's a simple union, generate the visit of the sole non-variant member inline. else: ret += mcgen(''' visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err); ''', 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) 3.a+b. Generate the error check for visit of non-variant members ret += gen_err_check(label='out_obj') 3.c. Generate visit of the active variant. ret += mcgen(''' if (!visit_start_union(v, !!(*obj)->data, &err) || err) { goto out_obj; } switch ((*obj)->%(c_name)s) { ''', # 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')) [Some stuff the patch doesn't change omitted...] out_obj: error_propagate(errp, err); err = NULL; if (*obj) { visit_end_union(v, !!(*obj)->data, &err); } error_propagate(errp, err); err = NULL; visit_end_struct(v, &err); out: error_propagate(errp, err); } ''') return ret Okay, the change to gen_visit_union() looks sane. Can we go one step further and generate and use visit_type_NAME_fields() even for simple unions?