Eric Blake <[email protected]> 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 <[email protected]> >> --- >> 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!
