| Message ID | 1444968943-11254-7-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 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 <eblake@redhat.com> > > --- > 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?
On 10/21/2015 11:36 AM, Markus Armbruster wrote: > 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 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 <eblake@redhat.com> >> >> +++ 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. I'm going to filter the re.search cleanups into its own prereq patch. But yes, it will need to care for empty base and no members (hmm, I really ought to add positive tests to qapi-schema-test for an empty inherited struct, to make sure I'm getting it right - even if we don't want that patch in the final series). > Diff is confusing (not your fault). Let me compare code before and > after real slow. I also plan for v10 to include a diff of the generated code in the commit message, if that will help make the change more obvious. > > = 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 where NAME is the union name. > 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) Could be made slightly simpler by generating these while we iterate over cases (but we'd have to be careful to generate into multiple variables, and then concat together at the end, since we can't generate one function in the body of the other). > > 3. Generate its visit_type_NAME(). > > > 3.a. If it's a flat union, generate the call of > visit_type_NAME_fields(). Not necessary for simple unions, see 1. Again, important to note that this was visit_type_UNION_fields(). > 3.b. Generate visit of discriminator. > > > 3.c. Generate visit of the active variant. > > = 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) Note that this creates visit_type_BASE_fields() (a different function). > > 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(). > > > 3.a. If it's a flat union, generate the call of > visit_type_NAME_fields(). Not done for simple unions, see 1. Again, now NAME is the base name rather than the union name. Subtle but important difference! > > 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. > > > 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. > > > Okay, the change to gen_visit_union() looks sane. Yes, you got it all correct. > > Can we go one step further and generate and use visit_type_NAME_fields() > even for simple unions? Not easily. Remember, for flat unions, we are calling visit_type_BASE_fields, not visit_type_UNION_fields. There is no base for a simple union. What I _am_ planning for future patches is the following: Change QAPISchemaObjectType for simple unions and alternates to set .local_members to the one-element implicit discriminator (right now, it is always an empty array, and we even assert that bool(.local_members) and bool(.variants) are mutually-exclusive in at least qapi-types.py visit_object_type()). Flat unions would keep .local_members as an empty array (there is no local member, just the inherited members from the base class, which includes the inherited discriminator). Then merge gen_visit_struct() and gen_visit_union() to look like: if base: # includes discriminator for flat union call visit_type_BASE_fields() for m in .local_members # includes discriminator for simple union call visit_type_MEMBER() if variants emit switch statement to visit each branch But if we want, that 'for m in .local_members' block can indeed be implemented via a call to visit_type_UNION_fields(), if that is any more efficient to implement. I guess it also boils down to deciding if visit_type_FOO_fields() should continue to be unconditional (either for all types, or at least for all types with non-empty .local_members).
Eric Blake <eblake@redhat.com> writes: > On 10/21/2015 11:36 AM, Markus Armbruster wrote: >> 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 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 <eblake@redhat.com> >>> > >>> +++ 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. > > I'm going to filter the re.search cleanups into its own prereq patch. > But yes, it will need to care for empty base and no members (hmm, I > really ought to add positive tests to qapi-schema-test for an empty > inherited struct, to make sure I'm getting it right - even if we don't > want that patch in the final series). Don't take my reluctance to take some positive tests as general opposition towards positive tests! Positive tests for corner cases like "empty base" are valuable. >> Diff is confusing (not your fault). Let me compare code before and >> after real slow. > > I also plan for v10 to include a diff of the generated code in the > commit message, if that will help make the change more obvious. > >> >> = 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 > > where NAME is the union name. > >> 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) > > Could be made slightly simpler by generating these while we iterate over > cases (but we'd have to be careful to generate into multiple variables, > and then concat together at the end, since we can't generate one > function in the body of the other). I doubt it would be an improvement. The loop is trivial, so de-duplicating it doesn't have much value. Having generator code arranged in the same order as the generated code *does* have value. >> 3. Generate its visit_type_NAME(). >> > >> >> 3.a. If it's a flat union, generate the call of >> visit_type_NAME_fields(). Not necessary for simple unions, see 1. > > Again, important to note that this was visit_type_UNION_fields(). > >> 3.b. Generate visit of discriminator. >> > >> >> 3.c. Generate visit of the active variant. >> > >> = 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) > > Note that this creates visit_type_BASE_fields() (a different function). Missed this detail, thanks. The old visit_type_UNION_fields() is visit_type_BASE_fields() less the tag visit. Reusing visit_type_BASE_fields() instead behaves as I described above, so my analysis remains valid regardless. visit_type_BASE_fields() should be generated when gen_visit_struct() processes BASE. Here, we should only generate a forward declaration, if necessary. >> >> 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(). >> > >> >> 3.a. If it's a flat union, generate the call of >> visit_type_NAME_fields(). Not done for simple unions, see 1. > > Again, now NAME is the base name rather than the union name. Subtle but > important difference! Indeed. Until now, having a separate visit_type_NAME_fields() function was pretty pointless: it was only called from visit_type_NAME(). Now, it's also called from visit_type_UNION() when NAME is UNION's base. Bonus: we don't duplicate the code visiting NAME's members into every union using it as base (this is what the commit message refers to when it says "it lets us share code"). Can we do the same for structs, please? The nice thing about generating code is that you don't have to copy/paste so much to get all the duplication your heart desires. >> >> 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. >> > >> >> 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. >> > >> >> Okay, the change to gen_visit_union() looks sane. > > Yes, you got it all correct. > >> >> Can we go one step further and generate and use visit_type_NAME_fields() >> even for simple unions? > > Not easily. Remember, for flat unions, we are calling > visit_type_BASE_fields, not visit_type_UNION_fields. There is no base > for a simple union. Scratch the idea, I was confused. > What I _am_ planning for future patches is the following: > > Change QAPISchemaObjectType for simple unions and alternates to set > .local_members to the one-element implicit discriminator (right now, it > is always an empty array, and we even assert that bool(.local_members) > and bool(.variants) are mutually-exclusive in at least qapi-types.py > visit_object_type()). Flat unions would keep .local_members as an empty > array (there is no local member, just the inherited members from the > base class, which includes the inherited discriminator). > > Then merge gen_visit_struct() and gen_visit_union() to look like: > > if base: > # includes discriminator for flat union > call visit_type_BASE_fields() > for m in .local_members > # includes discriminator for simple union > call visit_type_MEMBER() > if variants > emit switch statement to visit each branch > > But if we want, that 'for m in .local_members' block can indeed be > implemented via a call to visit_type_UNION_fields(), if that is any more > efficient to implement. I guess it also boils down to deciding if > visit_type_FOO_fields() should continue to be unconditional (either for > all types, or at least for all types with non-empty .local_members). Keeping the implicit tag implicit by not including it in local_members was a conscious design decision, but I'm quite open to revisit it. But perhaps we don't have to! We got simple unions because "first past the post": they solved immediate needs. When other needs arose, flat unions got bolted on. We kept simple unions because the bolted-on flat unions are bothersome to define. If we can fuse struct and union types into a reasonably pleasant variant record type, we may not need a simple union sugar anymore.
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: 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
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 <eblake@redhat.com> --- 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(-)