| Message ID | 1442872682-6523-11-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@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 <eblake@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 > #
On 09/28/2015 12:17 AM, Markus Armbruster wrote: > Eric Blake <eblake@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 then I shorten it later in 27/46, but the shorter form is consistent to all three due to this refactor into a common helper. > >> 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. Indeed - we got lucky that the output visitor's visit_optional() was a no-op. I'll make that fact more obvious in the commit message. >> >> -def gen_err_check(err): >> - if not err: >> - return '' >> - return mcgen(''' >> -if (%(err)s) { >> - goto out; >> -} >> -''', >> - err=err) >> - >> - > > Only code motion. I'm actually debating about splitting the move of this helper function into its own patch, and using it in more places. Part of my debate is that I'd rather go with: def gen_err_check(err='err', label='out'): if not err: return '' return mcgen(''' if (%(err)s) { goto %(label)s; } ''', err=err, label=label) so that it is applicable in more places, and so that callers don't have to worry about push_indent()/pop_indent() if it is at the default usage of 4 spaces (right now, all callers have to push, and not just callers at 8 spaces where it is embedded inside an 'if' block). Hmm, and just writing that, I'm wondering if we should fix mcgen() to eat leading whitespace on any final blank line [as a separate patch], since at least for me, emacs wants me to indent as: return mcgen(''' code ''', args) rather than with the closing ''' flush left. >> -''') >> + 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. Good idea, will work it into my v6.
Eric Blake <eblake@redhat.com> writes: > On 09/28/2015 12:17 AM, Markus Armbruster wrote: >> Eric Blake <eblake@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 then I shorten it later in 27/46, but the shorter form is consistent > to all three due to this refactor into a common helper. > >> >>> 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. > > Indeed - we got lucky that the output visitor's visit_optional() was a > no-op. I'll make that fact more obvious in the commit message. > > >>> >>> -def gen_err_check(err): >>> - if not err: >>> - return '' >>> - return mcgen(''' >>> -if (%(err)s) { >>> - goto out; >>> -} >>> -''', >>> - err=err) >>> - >>> - >> >> Only code motion. > > I'm actually debating about splitting the move of this helper function > into its own patch, and using it in more places. Part of my debate is > that I'd rather go with: > > def gen_err_check(err='err', label='out'): > if not err: > return '' > return mcgen(''' > if (%(err)s) { > goto %(label)s; > } > ''', > err=err, label=label) > > so that it is applicable in more places, and so that callers don't have > to worry about push_indent()/pop_indent() if it is at the default usage > of 4 spaces (right now, all callers have to push, and not just callers > at 8 spaces where it is embedded inside an 'if' block). Could have a parameter indent_amount with a suitable default, passed on to push_indent() / pop_indent(). Our helper function's indentation level is rather haphazard. > Hmm, and just writing that, I'm wondering if we should fix mcgen() to > eat leading whitespace on any final blank line [as a separate patch], > since at least for me, emacs wants me to indent as: > > return mcgen(''' > code > ''', args) > > rather than with the closing ''' flush left. I think Emacs is mistaken here. I find the indented ''' mildly distracting, because it immediately begs the question whether the trailing whitespace ends up in the generated code. If we want to add further processing, such as stripping trailing whitespace, the correct place is cgen(). If we decide to strip it, I guess we better strip it from every line, not just the last one. However, I think the appropriate place to strip whitespace is the source code. See also commit 77e703b. >>> -''') >>> + 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. > > Good idea, will work it into my v6.
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) - - 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) 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: - 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) - 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' + 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 #
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; |+ } |+ } 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 (has_offset) { | visit_type_int(v, &offset, "offset", &err); Signed-off-by: Eric Blake <eblake@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(-)