Re: [Qemu-devel] [PATCH RFC v2 19/47] qapi: Generated code cleanup

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4446 bytes --]

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    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]