Re: [Qemu-devel] [PATCH v2 15/19] qapi-visit: Move error check into gen_visit_members_call()

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
>
> ---
> 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?