| Message ID | 1435782155-31412-20-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/01/2015 02:22 PM, Markus Armbruster wrote: > Clean up white-space, brace placement, and superfluous Incomplete sentence. I bet it's because your editor line-wrapped, and the rest of your sentence was something like '#ifndef in a .c file.' (see [1] below), then git ate the line thinking it was a comment. I think I'm okay with cramming all of these cleanups into one patch rather than trying to do one style of cleanup per patch (blank lines, { placement, and guard cleanup), but the commit message could do a better job of explaining things. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > scripts/qapi-commands.py | 1 + > scripts/qapi-event.py | 3 +-- > scripts/qapi-types.py | 66 +++++++++++++++++++++++------------------------- > scripts/qapi-visit.py | 1 + > 4 files changed, 34 insertions(+), 37 deletions(-) Missing changes to docs/qapi-code-gen.txt to reflect the improvements. Since having docs that are stale compared to implementation can be misleading, I'd like to wait for v2 before giving my R-b; but see also [2] below. Overall, I'm a fan of the cleanups. I'm going to intersperse diffs of the generated files that were caused by some of these changes: > > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > index cfbd59c..223216d 100644 > --- a/scripts/qapi-commands.py > +++ b/scripts/qapi-commands.py > @@ -222,6 +222,7 @@ def gen_marshal_input(name, args, ret_type, middle_mode): > ret += mcgen(''' > > (void)args; > + > ''') This and similar hunts avoids extra blank lines. Not worth showing a diff to the generated files. > > ret += gen_sync_call(name, args, ret_type) > diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py > index 88b0620..7f238df 100644 > --- a/scripts/qapi-event.py > +++ b/scripts/qapi-event.py > @@ -167,8 +167,7 @@ extern const char *%(event_enum_name)s_lookup[]; > event_enum_name = event_enum_name) > > enum_decl = mcgen(''' > -typedef enum %(event_enum_name)s > -{ > +typedef enum %(event_enum_name)s { Several hunks like this; gets us generally closer to qemu style; with generated diffs like: qapi-types.h: -typedef struct int32List -{ +typedef struct int32List { union { int32_t value; uint64_t padding; > @@ -105,7 +103,8 @@ struct %(name)s > > def generate_enum_lookup(name, values): > ret = mcgen(''' > -const char * const %(name)s_lookup[] = { > + > +const char *const %(name)s_lookup[] = { [2] generated diffs like this: qapi-types.c: -const char * const OnOffAuto_lookup[] = { +const char *const OnOffAuto_lookup[] = { Hmm - we already failed to update docs/qapi-code-gen.txt in the past; we added a const in commit 2e4450ff that is missing from the documentation. > @@ -335,22 +333,22 @@ for typename in builtin_types.keys(): > fdecl.write(guardend("QAPI_TYPES_BUILTIN_STRUCT_DECL")) > > for expr in exprs: > - ret = "\n" > + ret = "" > if expr.has_key('struct'): > ret += generate_fwd_struct(expr['struct']) > elif expr.has_key('enum'): > - ret += generate_enum(expr['enum'], expr['data']) + "\n" > + ret += generate_enum(expr['enum'], expr['data']) > ret += generate_fwd_enum_struct(expr['enum']) > fdef.write(generate_enum_lookup(expr['enum'], expr['data'])) More work at avoiding extra blank lines. > @@ -370,34 +368,32 @@ fdecl.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DECL")) > # have the functions defined, so we use -b option to provide control > # over these cases > if do_builtins: > - fdef.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DEF")) > for typename in builtin_types.keys(): > fdef.write(generate_type_cleanup(typename + "List")) > - fdef.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DEF")) [1] this is the hunk whose description got corrupted in your commit message. It gives a diff like this: qapi-types.c: - -#ifndef QAPI_TYPES_BUILTIN_CLEANUP_DEF -#define QAPI_TYPES_BUILTIN_CLEANUP_DEF - - void qapi_free_int32List(int32List *obj) We conditionally declare the functions in the .h, but the .c is not compiled multiple times, so there is no need for a header-style guard.
Eric Blake <eblake@redhat.com> writes: > On 07/01/2015 02:22 PM, Markus Armbruster wrote: >> Clean up white-space, brace placement, and superfluous > > Incomplete sentence. I bet it's because your editor line-wrapped, and > the rest of your sentence was something like '#ifndef in a .c file.' > (see [1] below), then git ate the line thinking it was a comment. I > think I'm okay with cramming all of these cleanups into one patch rather > than trying to do one style of cleanup per patch (blank lines, { > placement, and guard cleanup), but the commit message could do a better > job of explaining things. Not throwing away half of it by accident would be a start :) >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> scripts/qapi-commands.py | 1 + >> scripts/qapi-event.py | 3 +-- >> scripts/qapi-types.py | 66 >> +++++++++++++++++++++++------------------------- >> scripts/qapi-visit.py | 1 + >> 4 files changed, 34 insertions(+), 37 deletions(-) > > Missing changes to docs/qapi-code-gen.txt to reflect the improvements. > Since having docs that are stale compared to implementation can be > misleading, I'd like to wait for v2 before giving my R-b; but see also > [2] below. I agree we should keep docs/qapi-code-gen.txt up-to-date. Since updating it is manual, updating it just once for a complete series can make sense. I'll double check it's accurate at the end of the series, then decide how to do necessary updates, if any. > Overall, I'm a fan of the cleanups. > > I'm going to intersperse diffs of the generated files that were caused > by some of these changes: > >> >> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py >> index cfbd59c..223216d 100644 >> --- a/scripts/qapi-commands.py >> +++ b/scripts/qapi-commands.py >> @@ -222,6 +222,7 @@ def gen_marshal_input(name, args, ret_type, middle_mode): >> ret += mcgen(''' >> >> (void)args; >> + >> ''') > > This and similar hunts avoids extra blank lines. Not worth showing a > diff to the generated files. > >> >> ret += gen_sync_call(name, args, ret_type) >> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py >> index 88b0620..7f238df 100644 >> --- a/scripts/qapi-event.py >> +++ b/scripts/qapi-event.py >> @@ -167,8 +167,7 @@ extern const char *%(event_enum_name)s_lookup[]; >> event_enum_name = event_enum_name) >> >> enum_decl = mcgen(''' >> -typedef enum %(event_enum_name)s >> -{ >> +typedef enum %(event_enum_name)s { > > Several hunks like this; gets us generally closer to qemu style; with > generated diffs like: > > qapi-types.h: > -typedef struct int32List > -{ > +typedef struct int32List { > union { > int32_t value; > uint64_t padding; > >> @@ -105,7 +103,8 @@ struct %(name)s >> >> def generate_enum_lookup(name, values): >> ret = mcgen(''' >> -const char * const %(name)s_lookup[] = { >> + >> +const char *const %(name)s_lookup[] = { > > [2] generated diffs like this: > > qapi-types.c: > -const char * const OnOffAuto_lookup[] = { > +const char *const OnOffAuto_lookup[] = { > > Hmm - we already failed to update docs/qapi-code-gen.txt in the past; we > added a const in commit 2e4450ff that is missing from the documentation. Minor review fail. Not the first time. >> @@ -335,22 +333,22 @@ for typename in builtin_types.keys(): >> fdecl.write(guardend("QAPI_TYPES_BUILTIN_STRUCT_DECL")) >> >> for expr in exprs: >> - ret = "\n" >> + ret = "" >> if expr.has_key('struct'): >> ret += generate_fwd_struct(expr['struct']) >> elif expr.has_key('enum'): >> - ret += generate_enum(expr['enum'], expr['data']) + "\n" >> + ret += generate_enum(expr['enum'], expr['data']) >> ret += generate_fwd_enum_struct(expr['enum']) >> fdef.write(generate_enum_lookup(expr['enum'], expr['data'])) > > More work at avoiding extra blank lines. > >> @@ -370,34 +368,32 @@ fdecl.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DECL")) >> # have the functions defined, so we use -b option to provide control >> # over these cases >> if do_builtins: >> - fdef.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DEF")) >> for typename in builtin_types.keys(): >> fdef.write(generate_type_cleanup(typename + "List")) >> - fdef.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DEF")) > > [1] this is the hunk whose description got corrupted in your commit > message. It gives a diff like this: > > qapi-types.c: > > - > -#ifndef QAPI_TYPES_BUILTIN_CLEANUP_DEF > -#define QAPI_TYPES_BUILTIN_CLEANUP_DEF > - > - > void qapi_free_int32List(int32List *obj) > > We conditionally declare the functions in the .h, but the .c is not > compiled multiple times, so there is no need for a header-style guard. This is going to help me reconstruct my crippled commit message, thanks!
Markus Armbruster <armbru@redhat.com> writes: > Eric Blake <eblake@redhat.com> writes: > >> On 07/01/2015 02:22 PM, Markus Armbruster wrote: [...] >>> @@ -105,7 +103,8 @@ struct %(name)s >>> >>> def generate_enum_lookup(name, values): >>> ret = mcgen(''' >>> -const char * const %(name)s_lookup[] = { >>> + >>> +const char *const %(name)s_lookup[] = { >> >> [2] generated diffs like this: >> >> qapi-types.c: >> -const char * const OnOffAuto_lookup[] = { >> +const char *const OnOffAuto_lookup[] = { >> >> Hmm - we already failed to update docs/qapi-code-gen.txt in the past; we >> added a const in commit 2e4450ff that is missing from the documentation. > > Minor review fail. Not the first time. I take that back, it's actually not visible in qapi-code-gen.txt. [...]
On 08/04/2015 03:08 AM, Markus Armbruster wrote: > Markus Armbruster <armbru@redhat.com> writes: > >> Eric Blake <eblake@redhat.com> writes: >> >>> On 07/01/2015 02:22 PM, Markus Armbruster wrote: > [...] >>>> @@ -105,7 +103,8 @@ struct %(name)s >>>> >>>> def generate_enum_lookup(name, values): >>>> ret = mcgen(''' >>>> -const char * const %(name)s_lookup[] = { >>>> + >>>> +const char *const %(name)s_lookup[] = { >>> >>> [2] generated diffs like this: >>> >>> qapi-types.c: >>> -const char * const OnOffAuto_lookup[] = { >>> +const char *const OnOffAuto_lookup[] = { >>> >>> Hmm - we already failed to update docs/qapi-code-gen.txt in the past; we >>> added a const in commit 2e4450ff that is missing from the documentation. >> >> Minor review fail. Not the first time. > > I take that back, it's actually not visible in qapi-code-gen.txt. Oh, you're right - the only mention of *_lookup[] in the docs file is the array generated for all events, and that one was output with correct spacing (until the rest of your series fixes it to share code rather than duplicate things with slight differences). Maybe we SHOULD be showing what the generators do for enums, unions, and alternates (by expanding the example-schema.json that is then fed to all the example script usage). But that's fine as a project for another day.
Eric Blake <eblake@redhat.com> writes: > On 08/04/2015 03:08 AM, Markus Armbruster wrote: >> Markus Armbruster <armbru@redhat.com> writes: >> >>> Eric Blake <eblake@redhat.com> writes: >>> >>>> On 07/01/2015 02:22 PM, Markus Armbruster wrote: >> [...] >>>>> @@ -105,7 +103,8 @@ struct %(name)s >>>>> >>>>> def generate_enum_lookup(name, values): >>>>> ret = mcgen(''' >>>>> -const char * const %(name)s_lookup[] = { >>>>> + >>>>> +const char *const %(name)s_lookup[] = { >>>> >>>> [2] generated diffs like this: >>>> >>>> qapi-types.c: >>>> -const char * const OnOffAuto_lookup[] = { >>>> +const char *const OnOffAuto_lookup[] = { >>>> >>>> Hmm - we already failed to update docs/qapi-code-gen.txt in the past; we >>>> added a const in commit 2e4450ff that is missing from the documentation. >>> >>> Minor review fail. Not the first time. >> >> I take that back, it's actually not visible in qapi-code-gen.txt. > > Oh, you're right - the only mention of *_lookup[] in the docs file is > the array generated for all events, and that one was output with correct > spacing (until the rest of your series fixes it to share code rather > than duplicate things with slight differences). > > Maybe we SHOULD be showing what the generators do for enums, unions, and > alternates (by expanding the example-schema.json that is then fed to all > the example script usage). But that's fine as a project for another day. The place where we try to exercise all the schema features is qapi-schema-test.json.
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index cfbd59c..223216d 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -222,6 +222,7 @@ def gen_marshal_input(name, args, ret_type, middle_mode): ret += mcgen(''' (void)args; + ''') ret += gen_sync_call(name, args, ret_type) diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py index 88b0620..7f238df 100644 --- a/scripts/qapi-event.py +++ b/scripts/qapi-event.py @@ -167,8 +167,7 @@ extern const char *%(event_enum_name)s_lookup[]; event_enum_name = event_enum_name) enum_decl = mcgen(''' -typedef enum %(event_enum_name)s -{ +typedef enum %(event_enum_name)s { ''', event_enum_name = event_enum_name) diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index c6c2786..4cc17da 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -15,8 +15,7 @@ from qapi import * def generate_fwd_builtin(name): return mcgen(''' -typedef struct %(name)sList -{ +typedef struct %(name)sList { union { %(type)s value; uint64_t padding; @@ -32,8 +31,7 @@ def generate_fwd_struct(name): typedef struct %(name)s %(name)s; -typedef struct %(name)sList -{ +typedef struct %(name)sList { union { %(name)s *value; uint64_t padding; @@ -45,8 +43,8 @@ typedef struct %(name)sList def generate_fwd_enum_struct(name): return mcgen(''' -typedef struct %(name)sList -{ + +typedef struct %(name)sList { union { %(name)s value; uint64_t padding; @@ -79,8 +77,8 @@ def generate_struct(expr): base = expr.get('base') ret = mcgen(''' -struct %(name)s -{ + +struct %(name)s { ''', name=c_name(structname)) @@ -105,7 +103,8 @@ struct %(name)s def generate_enum_lookup(name, values): ret = mcgen(''' -const char * const %(name)s_lookup[] = { + +const char *const %(name)s_lookup[] = { ''', name=c_name(name)) for value in values: @@ -119,7 +118,6 @@ const char * const %(name)s_lookup[] = { ret += mcgen(''' [%(max_index)s] = NULL, }; - ''', max_index=max_index) return ret @@ -127,13 +125,14 @@ const char * const %(name)s_lookup[] = { def generate_enum(name, values): name = c_name(name) lookup_decl = mcgen(''' -extern const char * const %(name)s_lookup[]; + +extern const char *const %(name)s_lookup[]; ''', name=name) enum_decl = mcgen(''' -typedef enum %(name)s -{ + +typedef enum %(name)s { ''', name=name) @@ -155,7 +154,7 @@ typedef enum %(name)s ''', name=name) - return lookup_decl + enum_decl + return enum_decl + lookup_decl def generate_alternate_qtypes(expr): @@ -163,6 +162,7 @@ def generate_alternate_qtypes(expr): members = expr['data'] ret = mcgen(''' + const int %(name)s_qtypes[QTYPE_MAX] = { ''', name=c_name(name)) @@ -198,8 +198,8 @@ def generate_union(expr, meta): discriminator_type_name = '%sKind' % (name) ret = mcgen(''' -struct %(name)s -{ + +struct %(name)s { ''', name=name) if base: @@ -317,14 +317,12 @@ fdef.write(mcgen(''' #include "qapi/dealloc-visitor.h" #include "%(prefix)sqapi-types.h" #include "%(prefix)sqapi-visit.h" - ''', prefix=prefix)) fdecl.write(mcgen(''' #include <stdbool.h> #include <stdint.h> - ''')) exprs = parse_schema(input_file) @@ -335,22 +333,22 @@ for typename in builtin_types.keys(): fdecl.write(guardend("QAPI_TYPES_BUILTIN_STRUCT_DECL")) for expr in exprs: - ret = "\n" + ret = "" if expr.has_key('struct'): ret += generate_fwd_struct(expr['struct']) elif expr.has_key('enum'): - ret += generate_enum(expr['enum'], expr['data']) + "\n" + ret += generate_enum(expr['enum'], expr['data']) ret += generate_fwd_enum_struct(expr['enum']) fdef.write(generate_enum_lookup(expr['enum'], expr['data'])) elif expr.has_key('union'): - ret += generate_fwd_struct(expr['union']) + "\n" + ret += generate_fwd_struct(expr['union']) enum_define = discriminator_find_enum_define(expr) if not enum_define: ret += generate_enum('%sKind' % expr['union'], expr['data'].keys()) fdef.write(generate_enum_lookup('%sKind' % expr['union'], expr['data'].keys())) elif expr.has_key('alternate'): - ret += generate_fwd_struct(expr['alternate']) + "\n" + ret += generate_fwd_struct(expr['alternate']) ret += generate_enum('%sKind' % expr['alternate'], expr['data'].keys()) fdef.write(generate_enum_lookup('%sKind' % expr['alternate'], expr['data'].keys())) @@ -370,34 +368,32 @@ fdecl.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DECL")) # have the functions defined, so we use -b option to provide control # over these cases if do_builtins: - fdef.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DEF")) for typename in builtin_types.keys(): fdef.write(generate_type_cleanup(typename + "List")) - fdef.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DEF")) for expr in exprs: - ret = "\n" + ret = "" if expr.has_key('struct'): ret += generate_struct(expr) + "\n" ret += generate_type_cleanup_decl(expr['struct'] + "List") - fdef.write(generate_type_cleanup(expr['struct'] + "List") + "\n") + fdef.write(generate_type_cleanup(expr['struct'] + "List")) ret += generate_type_cleanup_decl(expr['struct']) - fdef.write(generate_type_cleanup(expr['struct']) + "\n") + fdef.write(generate_type_cleanup(expr['struct'])) elif expr.has_key('union'): - ret += generate_union(expr, 'union') + ret += generate_union(expr, 'union') + "\n" ret += generate_type_cleanup_decl(expr['union'] + "List") - fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n") + fdef.write(generate_type_cleanup(expr['union'] + "List")) ret += generate_type_cleanup_decl(expr['union']) - fdef.write(generate_type_cleanup(expr['union']) + "\n") + fdef.write(generate_type_cleanup(expr['union'])) elif expr.has_key('alternate'): - ret += generate_union(expr, 'alternate') + ret += generate_union(expr, 'alternate') + "\n" ret += generate_type_cleanup_decl(expr['alternate'] + "List") - fdef.write(generate_type_cleanup(expr['alternate'] + "List") + "\n") + fdef.write(generate_type_cleanup(expr['alternate'] + "List")) ret += generate_type_cleanup_decl(expr['alternate']) - fdef.write(generate_type_cleanup(expr['alternate']) + "\n") + fdef.write(generate_type_cleanup(expr['alternate'])) elif expr.has_key('enum'): - ret += generate_type_cleanup_decl(expr['enum'] + "List") - fdef.write(generate_type_cleanup(expr['enum'] + "List") + "\n") + ret += "\n" + generate_type_cleanup_decl(expr['enum'] + "List") + fdef.write(generate_type_cleanup(expr['enum'] + "List")) else: continue fdecl.write(ret) diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 73f136f..427819a 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -62,6 +62,7 @@ def generate_visit_struct_fields(name, members, base = None): static void visit_type_%(name)s_fields(Visitor *m, %(name)s **obj, Error **errp) { Error *err = NULL; + ''', name=c_name(name)) push_indent()
Clean up white-space, brace placement, and superfluous Signed-off-by: Markus Armbruster <armbru@redhat.com> --- scripts/qapi-commands.py | 1 + scripts/qapi-event.py | 3 +-- scripts/qapi-types.py | 66 +++++++++++++++++++++++------------------------- scripts/qapi-visit.py | 1 + 4 files changed, 34 insertions(+), 37 deletions(-)