qemu-devel
[Top][All Lists]
Advanced

[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!



reply via email to

[Prev in Thread] Current Thread [Next in Thread]