Re: [Qemu-devel] [PATCH RFC v2 37/47] qapi: De-duplicate parameter list generation

[Eric Blake]

On 07/01/2015 02:22 PM, Markus Armbruster wrote:
> Generated qapi-event.[ch] lose line breaks.  No change otherwise.

For example,

-void qapi_event_send_block_image_corrupted(const char *device,
-                                           bool has_node_name,
-                                           const char *node_name,
-                                           const char *msg,
-                                           bool has_offset,
-                                           int64_t offset,
-                                           bool has_size,
-                                           int64_t size,
-                                           bool fatal,
-                                           Error **errp)
+void qapi_event_send_block_image_corrupted(const char *device, bool
has_node_name, const char *node_name, const char *msg, bool has_offset,
int64_t offset, bool has_size, int64_t size, bool fatal, Error **errp)

You know, I'd find it a bit more appealing if you had merged the
duplicate code in the _other_ direction. That is, qapi-event's wrapped
lines (usually) fit in 80 columns, and it would be nice if qapi-visit's
did the same.

Yeah, avoiding line wraps consumes fewer source bytes (fewer runs of
spaces), but the space isn't being wasted by storing generated files in
git, nor does the C compiler care which layout we use.  And honestly,
it's easier to spot changes in a vertical list than it is on a long
horizontal line, if a parameter gets added (or removed, although adding
is the more likely action with qapi).

> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  scripts/qapi-commands.py | 11 ++---------
>  scripts/qapi-event.py    | 18 +++---------------
>  scripts/qapi.py          | 16 ++++++++++++++++
>  3 files changed, 21 insertions(+), 24 deletions(-)

I'm a fan of de-duplication, so I'll review this on its merits; but I'm
omitting R-b on this round in hopes that you buy my argument to merge in
the other direction (make qapi-event's implementation the common one).

> 
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index d57f8d4..2dae425 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py

> -                 args=argstr)
> +                 params=gen_params(args, 'Error **errp'))

Caller 1.

> +++ b/scripts/qapi-event.py

> +    return 'void qapi_event_send_%(c_name)s(%(param)s)' % {
> +        'c_name': c_name(name.lower()),
> +        'param': gen_params(data, 'Error **errp')}

Caller 2.

>  
>  def gen_event_send_decl(name, data):
>      return mcgen('''
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 4d47214..c6a5ddc 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1384,6 +1384,22 @@ extern const char *const %(c_name)s_lookup[];
>                   c_name=c_name(name))
>      return ret
>  
> +def gen_params(args, extra):
> +    if not args:
> +        return extra

Both callers pass the same 'extra' - do you need it to be parameterized,
or can it just be generated as a constant here?  (I guess it depends on
what happens with the later introspection patch, which may become caller 3).

> +    assert not args.variants

This assert will trip if you don't fix events to reject 'data':'Union' :)

> +    ret = ""
> +    sep = ""
> +    for memb in args.members:
> +        ret += sep
> +        sep = ", "
> +        if memb.optional:
> +            ret += "bool has_%s, " % c_name(memb.name)

Didn't you just provide a patch that used '' rather than "" for all
generated C constructs?  This violates that paradigm.

> +        ret += "%s %s" % (memb.type.c_type(is_param=True), c_name(memb.name))
> +    if extra:
> +        ret += sep + extra
> +    return ret
> +

To produce line breaks, you could have to add a parameter so that
callers can pass in the starting column for each wrapped argument, and
then you'd have sep = ',\n' + ''.ljust(len).  Or even have the caller
choose its own separator (", " vs. ",\n    "), if you don't want to have
a diff in the generated output (but I think consistent generated output
is nicer).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature