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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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).

Number of source bytes is not an issue.

The generators make no effort to wrap source lines, except in the
qapi_event_send_FOO()'s parameter lists.

We could preserve that one-off.  We could extend it to more places that
can generate long lines, saddling the generation code with indentation
concerns.  I don't want to write such code, and I don't want to maintain
it.

Instead, why not keep the generators straightforward, and feed their
result to indent when "pretty" is wanted?  Requires an indent profile
matching QEMU style.

>> 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).

The series doesn't add callers later on.

I made it a parameter simply because I feel gen_params() shouldn't need
to know what extra parameters its caller may need.  Even when all
callers need the same.

>> +    assert not args.variants
>
> This assert will trip if you don't fix events to reject 'data':'Union' :)

Looks like it :)

>> +    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.

Will fix.

>> +        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).