Eric Blake <ebl...@redhat.com> writes:
> Consolidate the code between visit, command marshalling, and
> event generation that iterates over the members of a struct.
> It reduces code duplication in the generator, with no change to
> generated marshal code, slightly more verbose visit code:
>
> | visit_optional(v, &(*obj)->has_device, "device", &err);
> |- if (!err && (*obj)->has_device) {
> |- visit_type_str(v, &(*obj)->device, "device", &err);
> |- }
> | if (err) {
> | goto out;
> | }
> |+ if ((*obj)->has_device) {
> |+ visit_type_str(v, &(*obj)->device, "device", &err);
> |+ if (err) {
> |+ goto out;
> |+ }
> |+ }
I think the more verbose code is easier to understand, because it checks
for errors exactly the same way as we do all the time, mimimizing
cognitive load.
> and slightly more verbose event code (recall that the qmp
> output visitor has a no-op visit_optional()):
>
> |+ visit_optional(v, &has_offset, "offset", &err);
> |+ if (err) {
> |+ goto out;
> |+ }
If we had a written contract, I suspect not calling visit_optional()
would be a bug.
> | if (has_offset) {
> | visit_type_int(v, &offset, "offset", &err);
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
> scripts/qapi-commands.py | 38 +---------------------------------
> scripts/qapi-event.py | 35 +++-----------------------------
> scripts/qapi-visit.py | 26 +-----------------------
> scripts/qapi.py | 53
> ++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 58 insertions(+), 94 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 2151120..55287b1 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -25,17 +25,6 @@ def gen_command_decl(name, arg_type, ret_type):
> params=gen_params(arg_type, 'Error **errp'))
>
>
> -def gen_err_check(err):
> - if not err:
> - return ''
> - return mcgen('''
> -if (%(err)s) {
> - goto out;
> -}
> -''',
> - err=err)
> -
> -
Only code motion.
> def gen_call(name, arg_type, ret_type):
> ret = ''
>
> @@ -119,7 +108,6 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
> push_indent()
>
> if dealloc:
> - errparg = 'NULL'
> errarg = None
> ret += mcgen('''
> qmp_input_visitor_cleanup(mi);
> @@ -127,36 +115,12 @@ md = qapi_dealloc_visitor_new();
> v = qapi_dealloc_get_visitor(md);
> ''')
> else:
> - errparg = '&err'
> errarg = 'err'
> ret += mcgen('''
> v = qmp_input_get_visitor(mi);
> ''')
>
> - for memb in arg_type.members:
> - if memb.optional:
> - ret += mcgen('''
> -visit_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s);
> -''',
> - c_name=c_name(memb.name), name=memb.name,
> - errp=errparg)
> - ret += gen_err_check(errarg)
> - ret += mcgen('''
> -if (has_%(c_name)s) {
> -''',
> - c_name=c_name(memb.name))
> - push_indent()
> - ret += mcgen('''
> -visit_type_%(c_type)s(v, &%(c_name)s, "%(name)s", %(errp)s);
> -''',
> - c_name=c_name(memb.name), name=memb.name,
> - c_type=memb.type.c_name(), errp=errparg)
> - ret += gen_err_check(errarg)
> - if memb.optional:
> - pop_indent()
> - ret += mcgen('''
> -}
> -''')
> + ret += gen_visit_fields(arg_type.members, '', False, errarg)
Perhaps a bit neater: make parameters prefix='', need_cast=False, and
say prefix=... and need_cast=True in the one call where you need it.
>
> if dealloc:
> ret += mcgen('''
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index b43bbc2..6c70a06 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -74,38 +74,9 @@ def gen_event_send(name, arg_type):
>
> ''',
> name=name)
> -
> - for memb in arg_type.members:
> - if memb.optional:
Here's the missing visit_optional().
> - ret += mcgen('''
> - if (has_%(c_name)s) {
> -''',
> - c_name=c_name(memb.name))
> - push_indent()
> -
> - # Ugly: need to cast away the const
> - if memb.type.name == "str":
> - cast = '(char **)'
> - else:
> - cast = ''
> -
> - ret += mcgen('''
> - visit_type_%(c_type)s(v, %(cast)s&%(c_name)s, "%(name)s", &err);
> - if (err) {
> - goto out;
> - }
> -''',
> - cast=cast,
> - c_name=c_name(memb.name),
> - c_type=memb.type.c_name(),
> - name=memb.name)
> -
> - if memb.optional:
> - pop_indent()
> - ret += mcgen('''
> - }
> -''')
> -
> + push_indent()
> + ret += gen_visit_fields(arg_type.members, '', True, 'err')
> + pop_indent()
> ret += mcgen('''
>
> visit_end_struct(v, &err);
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 9c0328d..1f287ba 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -88,31 +88,7 @@ if (err) {
> ''',
> c_type=base.c_name(), c_name=c_name('base'))
>
> - for memb in members:
> - if memb.optional:
> - ret += mcgen('''
> -visit_optional(v, &(*obj)->has_%(c_name)s, "%(name)s", &err);
> -if (!err && (*obj)->has_%(c_name)s) {
> -''',
> - c_name=c_name(memb.name), name=memb.name)
Here's the unconventional error checking.
> - push_indent()
> -
> - ret += mcgen('''
> -visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
> -''',
> - c_type=memb.type.c_name(), c_name=c_name(memb.name),
> - name=memb.name)
> -
> - if memb.optional:
> - pop_indent()
> - ret += mcgen('''
> -}
> -''')
> - ret += mcgen('''
> -if (err) {
> - goto out;
> -}
> -''')
> + ret += gen_visit_fields(members, '(*obj)->', False, 'err')
>
> pop_indent()
> if re.search('^ *goto out;', ret, re.MULTILINE):
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 6f4e471..7275daa 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1531,6 +1531,59 @@ def gen_params(arg_type, extra):
> ret += sep + extra
> return ret
>
> +
> +def gen_err_check(err):
> + if not err:
> + return ''
> + return mcgen('''
> +if (%(err)s) {
> + goto out;
> +}
> +''',
> + err=err)
> +
> +
> +def gen_visit_fields(members, prefix, need_cast, errarg):
> + ret = ''
> + if errarg:
> + errparg = '&' + errarg
> + else:
> + errparg = 'NULL'
Suggest a blank line here, just like in the code you replace.
> + for memb in members:
> + if memb.optional:
> + ret += mcgen('''
> +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(errarg)
> + ret += mcgen('''
> +if (%(prefix)shas_%(c_name)s) {
> +''',
> + prefix=prefix, c_name=c_name(memb.name))
> + push_indent()
> +
> + # Ugly: sometimes we need to cast away const
> + if need_cast and memb.type.name == 'str':
> + cast = '(char **)'
> + else:
> + cast = ''
> +
> + ret += mcgen('''
> +visit_type_%(c_type)s(v, %(cast)s&%(prefix)s%(c_name)s, "%(name)s",
> %(errp)s);
> +''',
> + 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(errarg)
> +
> + if memb.optional:
> + pop_indent()
> + ret += mcgen('''
> +}
> +''')
> + return ret
> +
> #
> # Common command line parsing
> #
