Eric Blake <ebl...@redhat.com> 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 <arm...@redhat.com>
>> ---
>> 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).
