Re: [Qemu-devel] [RFC PATCH v7 17/18] qapi: Simplify gen_visit_fields() error handling

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Since we have consolidated all generated code to use 'err' as
> the name of the local variable for error detection, we can
> simplify the decision on whether to skip error detection (useful
> for deallocation paths) to be a boolean.
>
> This requires the python 2.5 ternary syntax.

Let's drop this sentence.

> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v7: new patch; rfc as it depends on Markus' configure change to
> require python 2.6
> ---
>  scripts/qapi-commands.py |  4 +---
>  scripts/qapi.py          | 23 ++++++++++-------------
>  2 files changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 9d214a6..43a893b 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -101,19 +101,17 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
>          return ret
>
>      if dealloc:
> -        errarg = None
>          ret += mcgen('''
>      qmp_input_visitor_cleanup(qiv);
>      qdv = qapi_dealloc_visitor_new();
>      v = qapi_dealloc_get_visitor(qdv);
>  ''')
>      else:
> -        errarg = 'err'
>          ret += mcgen('''
>      v = qmp_input_get_visitor(qiv);
>  ''')
>
> -    ret += gen_visit_fields(arg_type.members, errarg=errarg)
> +    ret += gen_visit_fields(arg_type.members, skiperr=dealloc)
>
>      if dealloc:
>          ret += mcgen('''
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index ada6380..7761b4b 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1537,23 +1537,19 @@ def gen_params(arg_type, extra):
>      return ret
>
>
> -def gen_err_check(err='err', label='out'):
> -    if not err:
> +def gen_err_check(label='out', skiperr=False):
> +    if skiperr:
>          return ''
>      return mcgen('''
> -    if (%(err)s) {
> +    if (err) {
>          goto %(label)s;
>      }
>  ''',
> -                 err=err, label=label)
> +                 label=label)
>
>
> -def gen_visit_fields(members, prefix='', need_cast=False, errarg='err'):
> +def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
>      ret = ''
> -    if errarg:
> -        errparg = '&' + errarg
> -    else:
> -        errparg = 'NULL'
>
>      for memb in members:
>          if memb.optional:
> @@ -1561,8 +1557,9 @@ def gen_visit_fields(members, prefix='', 
> need_cast=False, errarg='err'):
>      visit_optional(v, &%(prefix)shas_%(c_name)s, "%(name)s", %(errp)s);
>  ''',
>                           prefix=prefix, c_name=c_name(memb.name),
> -                         name=memb.name, errp=errparg)
> -            ret += gen_err_check(err=errarg)
> +                         name=memb.name,
> +                         errp='&err' if not skiperr else 'NULL')
> +            ret += gen_err_check(skiperr=skiperr)
>              ret += mcgen('''
>      if (%(prefix)shas_%(c_name)s) {
>  ''',
> @@ -1580,8 +1577,8 @@ def gen_visit_fields(members, prefix='', 
> need_cast=False, errarg='err'):
>  ''',
>                       c_type=memb.type.c_name(), prefix=prefix, cast=cast,
>                       c_name=c_name(memb.name), name=memb.name,
> -                     errp=errparg)
> -        ret += gen_err_check(err=errarg)
> +                     errp='&err' if not skiperr else 'NULL')
> +        ret += gen_err_check(skiperr=skiperr)
>
>          if memb.optional:
>              pop_indent()

I'm not afraid of ternaries, but I guess I would've gone for the minimal
change here, i.e. something like:
 
-def gen_visit_fields(members, prefix='', need_cast=False, errarg='err'):
+def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
     ret = ''
-    if errarg:
-        errparg = '&' + errarg
-    else:
+    if skiperr:
         errparg = 'NULL'
+    else:
+        errparg = '&err'
 
     for memb in members:
         if memb.optional:
@@ -1562,7 +1562,7 @@ def gen_visit_fields(members, prefix='', need_cast=False, 
errarg='err'):
 ''',
                          prefix=prefix, c_name=c_name(memb.name),
                          name=memb.name, errp=errparg)
-            ret += gen_err_check(err=errarg)
+            ret += gen_err_check(skiperr=skiperr)
             ret += mcgen('''
     if (%(prefix)shas_%(c_name)s) {
 ''',
@@ -1581,7 +1581,7 @@ def gen_visit_fields(members, prefix='', need_cast=False, 
errarg='err'):
                      c_type=memb.type.c_name(), prefix=prefix, cast=cast,
                      c_name=c_name(memb.name), name=memb.name,
                      errp=errparg)
-        ret += gen_err_check(err=errarg)
+        ret += gen_err_check(skiperr=skiperr)
 
         if memb.optional:
             pop_indent()

Matter of taste.