Eric Blake <ebl...@redhat.com> writes:
> When first introduced, neither branch of gen_visit_members_call()
> would output a goto. But now that the implicit struct visit
> always ends with a goto, we should do the same for regular
> struct visits, so that callers don't have to worry about whether
> they are creating two identical goto's in a row.
>
> Generated code gets slightly larger; if desired, we could patch
> qapi.py:gen_visit_members() to have a mode where it skips the
> final goto and leave it up to the callers when to use that mode,
> but that adds more maintenance burden when the compiler should
> be smart enough to not bloat the .o file just because the .c
> file got larger.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
> ---
> v2: rebase onto s/fields/members/ change
> ---
> scripts/qapi-visit.py | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index e281d21..a17ecc1 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -48,6 +48,7 @@ def gen_visit_members_call(typ, direct_name,
> implicit_name=None):
assert isinstance(typ, QAPISchemaObjectType)
if typ.is_empty():
pass
elif typ.is_implicit():
assert implicit_name
assert not typ.variants
ret += gen_visit_members(typ.members, prefix=implicit_name)
This is the goto-generating part mentioned in the commit message.
else:
ret += mcgen('''
> visit_type_%(c_type)s_members(v, %(c_name)s, &err);
> ''',
> c_type=typ.c_name(), c_name=direct_name)
> + ret += gen_err_check()
> return ret
Emitting the gen_err_check() right after emitting the call it checks
makes for simpler and more robust code.
>
>
> @@ -63,7 +64,6 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s
> *obj, Error **errp)
>
> if base:
> ret += gen_visit_members_call(base, '(%s *)obj' % base.c_name())
> - ret += gen_err_check()
>
> ret += gen_visit_members(members, prefix='obj->')
>
Adding more context to show the other use of gen_visit_members_call():
if variants:
ret += mcgen('''
switch (obj->%(c_name)s) {
''',
c_name=c_name(variants.tag_member.name))
for var in variants.variants:
ret += mcgen('''
case %(case)s:
''',
case=c_enum_const(variants.tag_member.type.name,
var.name,
variants.tag_member.type.prefix))
push_indent()
ret += gen_visit_members_call(var.type,
'&obj->u.' + c_name(var.name),
'obj->u.' + c_name(var.name) + '.')
This is where the generated code grows. Before the patch, we sometimes
omit the goto on error, which works, because the label comes right after
the switch.
pop_indent()
ret += mcgen('''
break;
''')
ret += mcgen('''
default:
abort();
}
''')
I wonder whether it would be simpler to make gen_visit_members_call()
add the goto from the start.
> @@ -95,9 +95,9 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s
> *obj, Error **errp)
> }
> ''')
>
> - # 'goto out' produced for base, by gen_visit_members() for each member,
> - # and if variants were present
> - if base or members or variants:
> + # 'goto out' produced for non-empty base, by gen_visit_members() for
> + # each member, and if variants were present
> + if (base and not base.is_empty()) or members or variants:
> ret += mcgen('''
>
> out:
Uh, sure this hunk belongs to this patch?
