| Message ID | 1435782155-31412-38-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <armbru@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). > + 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> 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 <armbru@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).
On 07/28/2015 05:15 AM, Markus Armbruster wrote: >> 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. Long lines aren't the end of the world. They may be harder to read when diffing pre- and post-patch generated output to see if a generator change makes sense, but you have a point that line wrapping is more maintenance. So you win; keep the long lines, and if someone wants wrapping, they can (re-)add it as a later patch series.
Eric Blake <eblake@redhat.com> writes: > On 07/28/2015 05:15 AM, Markus Armbruster wrote: > >>> 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. > > Long lines aren't the end of the world. They may be harder to read when > diffing pre- and post-patch generated output to see if a generator > change makes sense, but you have a point that line wrapping is more > maintenance. So you win; keep the long lines, and if someone wants > wrapping, they can (re-)add it as a later patch series. Settled. Aside: diff-mode can color the parts of the changed lines that actually differ. Indispensable, and not just for long lines.
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 @@ -16,19 +16,12 @@ from qapi import * import re def gen_command_decl(name, args, rets): - argstr = '' - if args: - for memb in args.members: - if memb.optional: - argstr += 'bool has_%s, ' % c_name(memb.name) - argstr += '%s %s, ' % (memb.type.c_type(is_param=True), - c_name(memb.name)) return mcgen(''' -%(c_type)s qmp_%(c_name)s(%(args)sError **errp); +%(c_type)s qmp_%(c_name)s(%(params)s); ''', c_type=(rets and rets.c_type()) or 'void', c_name=c_name(name), - args=argstr) + params=gen_params(args, 'Error **errp')) def gen_err_check(err): if not err: diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py index 03bb1ec..184a81f 100644 --- a/scripts/qapi-event.py +++ b/scripts/qapi-event.py @@ -14,21 +14,9 @@ from qapi import * def gen_event_send_proto(name, data): - api_name = "void qapi_event_send_%s(" % c_name(name).lower() - l = len(api_name) - - if data: - for m in data.members: - if m.optional: - api_name += "bool has_%s,\n" % c_name(m.name) - api_name += "".ljust(l) - - api_name += "%s %s,\n" % (m.type.c_type(is_param=True), - c_name(m.name)) - api_name += "".ljust(l) - - api_name += "Error **errp)" - return api_name + return 'void qapi_event_send_%(c_name)s(%(param)s)' % { + 'c_name': c_name(name.lower()), + 'param': gen_params(data, 'Error **errp')} 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 + assert not args.variants + ret = "" + sep = "" + for memb in args.members: + ret += sep + sep = ", " + if memb.optional: + ret += "bool has_%s, " % c_name(memb.name) + ret += "%s %s" % (memb.type.c_type(is_param=True), c_name(memb.name)) + if extra: + ret += sep + extra + return ret + # # Common command line parsing #
Generated qapi-event.[ch] lose line breaks. No change otherwise. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- scripts/qapi-commands.py | 11 ++--------- scripts/qapi-event.py | 18 +++--------------- scripts/qapi.py | 16 ++++++++++++++++ 3 files changed, 21 insertions(+), 24 deletions(-)