From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43963) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZK8Y2-0006RR-JG for qemu-devel@nongnu.org; Tue, 28 Jul 2015 13:20:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZK8Xv-0000hp-CW for qemu-devel@nongnu.org; Tue, 28 Jul 2015 13:20:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40241) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZK8Xv-0000gm-4o for qemu-devel@nongnu.org; Tue, 28 Jul 2015 13:20:07 -0400 From: Markus Armbruster References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-38-git-send-email-armbru@redhat.com> <55B14005.9090909@redhat.com> Date: Tue, 28 Jul 2015 13:15:04 +0200 In-Reply-To: <55B14005.9090909@redhat.com> (Eric Blake's message of "Thu, 23 Jul 2015 13:27:01 -0600") Message-ID: <87twsown2f.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH RFC v2 37/47] qapi: De-duplicate parameter list generation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, berto@igalia.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com Eric Blake 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 >> --- >> 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).