[v10,04/13] qapi: Drop pointless gotos in generated code

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 <eblake@redhat.com>

---
v10: new patch
---
 scripts/qapi-event.py |  7 +++++--
 scripts/qapi-visit.py | 10 ++++++----
 scripts/qapi.py       |  8 ++++++--
 3 files changed, 17 insertions(+), 8 deletions(-)

Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
>
> ---
> 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?

On 02/16/2016 09:27 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
>>

>> @@ -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?

I'm fine with dropping 3 and 4/13 if you think they don't add anything
(and they do indeed make it more complicated to reason about when it is
safe to drop a goto, and furthermore when to omit the label because all
gotos were dropped).  The generated code will occupy more source code
bytes, but as you say, the optimizing compiler should not be bloating
the code any differently based on whether the goto is present or absent.

Okay, just in typing that, you've convinced me - ease of maintenance
should triumph over concise generated code.