Eric Blake <ebl...@redhat.com> writes:
> qapi-commands had a nice helper gen_err_check(), but did not
In fact, it still has :)
> use it everywhere. In fact, using it in more places makes it
> easier to reduce the lines of code used in appending an error
Suggest "used for generating error checks"
> check in generated code (previously required a multi-line
> mcgen(), which didn't add any use of parameterization), which
> in turn makes it easier to write the next patch which will
> consolidate another common pattern among the generators.
I think we should burn some of the whiches ;)
Drop the paranthesis?
> The diffstat of this patch doesn't quite show as big a
> reduction in lines as I had hoped, but that is in part due to
Almost none...
> the duplication of some FIXME comments.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
Have you diffed the generated code before and after the patch?
> ---
> v6: new, split off of 10/46, add label parameter, and catch more
> instances that could use it
> ---
> scripts/qapi-commands.py | 29 +++++++-------------
> scripts/qapi-event.py | 16 +++++------
> scripts/qapi-visit.py | 69
> ++++++++++++++++++++++++------------------------
> scripts/qapi.py | 12 +++++++++
> 4 files changed, 62 insertions(+), 64 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 1d3c0d5..713aa0b 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)
> -
> -
> def gen_call(name, arg_type, ret_type):
> ret = ''
>
> @@ -56,7 +45,7 @@ def gen_call(name, arg_type, ret_type):
> ''',
> c_name=c_name(name), args=argstr, lhs=lhs)
> if ret_type:
> - ret += gen_err_check('err')
> + ret += gen_err_check()
> ret += mcgen('''
>
> qmp_marshal_output_%(c_name)s(retval, ret, &err);
> @@ -133,7 +122,7 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
> ''',
> c_name=c_name(memb.name), name=memb.name,
> errp=errparg)
> - ret += gen_err_check(errarg)
> + ret += gen_err_check(err=errarg)
> ret += mcgen('''
> if (has_%(c_name)s) {
> ''',
> @@ -144,7 +133,7 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
> ''',
> c_name=c_name(memb.name), name=memb.name,
> c_type=memb.type.c_name(), errp=errparg)
> - ret += gen_err_check(errarg)
> + ret += gen_err_check(err=errarg)
> if memb.optional:
> pop_indent()
> ret += mcgen('''
> @@ -159,7 +148,7 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
>
>
> def gen_marshal_output(ret_type):
> - return mcgen('''
> + ret = mcgen('''
>
> static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject
> **ret_out, Error **errp)
> {
> @@ -170,9 +159,10 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s
> ret_in, QObject **ret_out,
>
> v = qmp_output_get_visitor(qov);
> visit_type_%(c_name)s(v, &ret_in, "unused", &err);
> - if (err) {
> - goto out;
> - }
> +''',
> + c_type=ret_type.c_type(), c_name=ret_type.c_name())
> + ret += gen_err_check()
> + ret += mcgen('''
> *ret_out = qmp_output_get_qobject(qov);
>
> out:
Here's a case that becomes more verbose, so it's not just comment
duplication. Also becomes a bit harder to read, I think.
> @@ -184,7 +174,8 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s
> ret_in, QObject **ret_out,
> qapi_dealloc_visitor_cleanup(qdv);
> }
> ''',
> - c_type=ret_type.c_type(), c_name=ret_type.c_name())
> + c_name=ret_type.c_name())
> + return ret
>
>
> def gen_marshal_proto(name):
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index b43bbc2..bbc6169 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -68,12 +68,10 @@ def gen_event_send(name, arg_type):
>
> /* Fake visit, as if all members are under a structure */
> visit_start_struct(v, NULL, "", "%(name)s", 0, &err);
> - if (err) {
> - goto out;
> - }
> -
> ''',
> name=name)
> + ret += gen_err_check()
> + ret += '\n'
>
> for memb in arg_type.members:
> if memb.optional:
This one gets marginally shorter, but hardly simpler.
> @@ -91,14 +89,12 @@ def gen_event_send(name, arg_type):
>
> 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)
> + ret += gen_err_check()
>
> if memb.optional:
> pop_indent()
> @@ -109,9 +105,9 @@ def gen_event_send(name, arg_type):
> ret += mcgen('''
>
> visit_end_struct(v, &err);
> - if (err) {
> - goto out;
> - }
> +''')
> + ret += gen_err_check()
> + ret += mcgen('''
>
> obj = qmp_output_get_qobject(qov);
> g_assert(obj != NULL);
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 7ce8616..fec07ad 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -81,11 +81,9 @@ static void visit_type_%(c_name)s_fields(Visitor *v,
> %(c_name)s **obj, Error **e
> if base:
> ret += mcgen('''
> visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
> - if (err) {
> - goto out;
> - }
> ''',
> c_type=base.c_name(), c_name=c_name('base'))
> + ret += gen_err_check()
>
> for memb in members:
> if memb.optional:
> @@ -107,11 +105,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v,
> %(c_name)s **obj, Error **e
> ret += mcgen('''
> }
> ''')
> - ret += mcgen('''
> - if (err) {
> - goto out;
> - }
> -''')
> + ret += gen_err_check()
>
> if re.search('^ *goto out;', ret, re.MULTILINE):
> ret += mcgen('''
> @@ -154,7 +148,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj,
> const char *name, Error
>
>
> def gen_visit_list(name, element_type):
> - return mcgen('''
> + ret = mcgen('''
>
> void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name,
> Error **errp)
> {
> @@ -162,9 +156,10 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj,
> const char *name, Error
> GenericList *i, **prev;
>
> visit_start_list(v, name, &err);
> - if (err) {
> - goto out;
> - }
> +''',
> + c_name=c_name(name), c_elt_type=element_type.c_name())
> + ret += gen_err_check()
> + ret += mcgen('''
>
> for (prev = (GenericList **)obj;
> !err && (i = visit_next_list(v, prev, &err)) != NULL;
> @@ -181,6 +176,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj,
> const char *name, Error
> }
> ''',
> c_name=c_name(name), c_elt_type=element_type.c_name())
> + return ret
>
>
> def gen_visit_enum(name):
> @@ -202,16 +198,17 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s
> **obj, const char *name, Error
> Error *err = NULL;
>
> visit_start_implicit_struct(v, (void**) obj, sizeof(%(c_name)s), &err);
> - if (err) {
> - goto out;
> - }
> +''',
> + c_name=c_name(name))
> + ret += gen_err_check()
> + ret += mcgen('''
> visit_get_next_type(v, (int*) &(*obj)->kind, %(c_name)s_qtypes, name,
> &err);
> - if (err) {
> - goto out_obj;
> - }
> +''',
> + c_name=c_name(name))
> + ret += gen_err_check(label='out_obj')
> + ret += mcgen('''
> switch ((*obj)->kind) {
> -''',
> - c_name=c_name(name))
> +''')
>
> for var in variants.variants:
> ret += mcgen('''
> @@ -259,23 +256,21 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s
> **obj, const char *name, Error
> Error *err = NULL;
>
> visit_start_struct(v, (void **)obj, "%(name)s", name,
> sizeof(%(c_name)s), &err);
> - if (err) {
> - goto out;
> - }
> +''',
> + c_name=c_name(name), name=name)
> + ret += gen_err_check()
> + ret += mcgen('''
> if (!*obj) {
> goto out_obj;
> }
> -''',
> - c_name=c_name(name), name=name)
> +''')
>
> if base:
> ret += mcgen('''
> visit_type_%(c_name)s_fields(v, obj, &err);
> - if (err) {
> - goto out_obj;
> - }
> ''',
> c_name=c_name(name))
> + ret += gen_err_check(label='out_obj')
>
> tag_key = variants.tag_member.name
> if not variants.tag_name:
> @@ -283,13 +278,6 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj,
> const char *name, Error
> tag_key = 'type'
> ret += mcgen('''
> visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
> - if (err) {
> - goto out_obj;
> - }
> - if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
> - goto out_obj;
> - }
> - switch ((*obj)->%(c_name)s) {
> ''',
> c_type=variants.tag_member.type.c_name(),
> # TODO ugly special case for simple union
> @@ -297,6 +285,17 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj,
> const char *name, Error
> # it, then: c_name=c_name(variants.tag_member.name)
> c_name=c_name(variants.tag_name or 'kind'),
> name=tag_key)
> + ret += gen_err_check(label='out_obj')
> + ret += mcgen('''
> + if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
> + goto out_obj;
> + }
> + switch ((*obj)->%(c_name)s) {
> +''',
> + # TODO ugly special case for simple union
> + # Use same tag name in C as on the wire to get rid of
> + # it, then: c_name=c_name(variants.tag_member.name)
> + c_name=c_name(variants.tag_name or 'kind'))
>
> for var in variants.variants:
> # TODO ugly special case for simple union
Here's the duplicated comment. It's a TODO, not a FIXME.
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 4825e62..0593b71 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1536,6 +1536,18 @@ def gen_params(arg_type, extra):
> ret += sep + extra
> return ret
>
> +
> +def gen_err_check(err='err', label='out'):
> + if not err:
> + return ''
> + return mcgen('''
> + if (%(err)s) {
> + goto %(label)s;
> + }
> +''',
> + err=err, label=label)
> +
> +
> #
> # Common command line parsing
> #
I don't know. Could be worthwhile if it really makes further work
easier.
To really cut the verbosity, I figure we'd have to do something more
radical, like having cgen() recognize a (short!) pattern and replace it
with a full-blown error check. Not sure that's actually a good idea,
though :)
