[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC v2 19/47] qapi: Generated code cleanup
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH RFC v2 19/47] qapi: Generated code cleanup |
Date: |
Mon, 27 Jul 2015 10:07:36 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> 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 <address@hidden>
>> ---
>> 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!
- Re: [Qemu-devel] [PATCH RFC v2 41/47] qom: Don't use 'gen': false for qom-get, qom-set, object-add, (continued)
[Qemu-devel] [PATCH RFC v2 19/47] qapi: Generated code cleanup, Markus Armbruster, 2015/07/01
[Qemu-devel] [PATCH RFC v2 45/47] qapi: New QMP command query-schema for QMP schema introspection, Markus Armbruster, 2015/07/01
- Re: [Qemu-devel] [PATCH RFC v2 45/47] qapi: New QMP command query-schema for QMP schema introspection, Eric Blake, 2015/07/23
- Re: [Qemu-devel] [PATCH RFC v2 45/47] qapi: New QMP command query-schema for QMP schema introspection, Markus Armbruster, 2015/07/28
- Re: [Qemu-devel] [PATCH RFC v2 45/47] qapi: New QMP command query-schema for QMP schema introspection, Eric Blake, 2015/07/28
- Re: [Qemu-devel] [PATCH RFC v2 45/47] qapi: New QMP command query-schema for QMP schema introspection, Markus Armbruster, 2015/07/29
- Re: [Qemu-devel] [PATCH RFC v2 45/47] qapi: New QMP command query-schema for QMP schema introspection, Eric Blake, 2015/07/29
- Re: [Qemu-devel] [PATCH RFC v2 45/47] qapi: New QMP command query-schema for QMP schema introspection, Markus Armbruster, 2015/07/29
[Qemu-devel] [PATCH RFC v2 44/47] qapi: Pseudo-type '**' is now unused, drop it, Markus Armbruster, 2015/07/01