Re: [Qemu-devel] [PATCH v10 04/13] qapi: Drop pointless gotos in generated code

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> There's no point in emitting a goto if the very next thing
> emitted will be the label.  A bit of cleverness in gen_visit_fields()
> will let us choose when to omit a final error check (basically,
> swap the order of emitted text within the loop, with the error
> check omitted on the first pass, then checking whether to emit a
> final check after the loop); and in turn omit unused labels.
>
> The change to generated code is a bit easier because we split out
> the reindentation of the remaining gotos in the previous patch.
> For example, in visit_type_ChardevReturn_fields():
>
> |     if (visit_optional(v, "pty", &obj->has_pty)) {
> |         visit_type_str(v, "pty", &obj->pty, &err);
> |     }
> |-    if (err) {
> |-        goto out;
> |-    }
> |-
> |-out:
> |     error_propagate(errp, err);
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v10: new patch
> ---
>  scripts/qapi-event.py |  7 +++++--
>  scripts/qapi-visit.py | 10 ++++++----
>  scripts/qapi.py       |  8 ++++++--
>  3 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index 544ae12..b50ac29 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -68,9 +68,12 @@ def gen_event_send(name, arg_type):
>                       name=name)
>          ret += gen_err_check()
>          ret += gen_visit_fields(arg_type.members, need_cast=True,
> -                                label='out_obj')
> -        ret += mcgen('''
> +                                label='out_obj', skiplast=True)
> +        if len(arg_type.members) > 1:
> +            ret += mcgen('''
>  out_obj:
> +''')
> +        ret += mcgen('''
>      visit_end_struct(v, err ? NULL : &err);
>      if (err) {
>          goto out;
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 0cc9b08..0be396b 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -92,12 +92,14 @@ static void visit_type_%(c_name)s_fields(Visitor *v, 
> %(c_name)s **obj, Error **e
>      visit_type_%(c_type)s_fields(v, (%(c_type)s **)obj, &err);
>  ''',
>                       c_type=base.c_name())
> -        ret += gen_err_check()
> +        if members:
> +            ret += gen_err_check()
>
> -    ret += gen_visit_fields(members, prefix='(*obj)->')
> +    ret += gen_visit_fields(members, prefix='(*obj)->', skiplast=True)
>
> -    # 'goto out' produced for base, and by gen_visit_fields() for each member
> -    if base or members:
> +    # 'goto out' produced for base, and by gen_visit_fields() for each
> +    # member except the last
> +    if bool(base) + len(members) > 1:
>          ret += mcgen('''
>
>  out:
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index fab5001..4d75d75 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1645,14 +1645,17 @@ def gen_err_check(label='out', skiperr=False):
>
>
>  def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False,
> -                     label='out'):
> +                     label='out', skiplast=False):
>      ret = ''
>      if skiperr:
>          errparg = 'NULL'
>      else:
>          errparg = '&err'
> +    local_skiperr = True
>
>      for memb in members:
> +        ret += gen_err_check(skiperr=local_skiperr, label=label)
> +        local_skiperr = skiperr
>          if memb.optional:
>              ret += mcgen('''
>      if (visit_optional(v, "%(name)s", &%(prefix)shas_%(c_name)s)) {
> @@ -1679,8 +1682,9 @@ def gen_visit_fields(members, prefix='', 
> need_cast=False, skiperr=False,
>              ret += mcgen('''
>      }
>  ''')
> +
> +    if members and not skiplast:
>          ret += gen_err_check(skiperr=skiperr, label=label)
> -
>      return ret

Is saving a goto the compiler can easily eliminate worth complicating
the code?