Re: [Qemu-devel] [PATCH RFC v2 33/47] qapi: Clean up after recent conversions to QAPISchemaVisitor

[Eric Blake <eblake@redhat.com>]

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

On 07/01/2015 02:22 PM, Markus Armbruster wrote:
> Drop helper functions that are now unused.
> 
> Make pylint reasonably happy.
> 
> Rename generate_FOO() functions to gen_FOO() for consistency.
> 
> Use more consistent and sensible variable names.
> 
> Consistently use c_ for mapping keys when their value is a C
> identifier or type.
> 
> Simplify gen_enum() and gen_visit_union()
> 
> Consistently use single quotes for C text string literals.

Not sure if it is worth splitting this into pieces.  Fortunately, there
are no changes to generated output.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-commands.py | 109 +++++++++++++++++++------------------
>  scripts/qapi-event.py    | 117 +++++++++++++++++++---------------------
>  scripts/qapi-types.py    |  68 ++++++++++++-----------
>  scripts/qapi-visit.py    | 121 ++++++++++++++++++++---------------------
>  scripts/qapi.py          | 138 +++++++++--------------------------------------
>  5 files changed, 229 insertions(+), 324 deletions(-)
> 
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index d3bddb6..5d11032 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -15,20 +15,20 @@
>  from qapi import *
>  import re
>  
> -def generate_command_decl(name, args, ret_type):
> -    arglist=""
> +def gen_command_decl(name, args, rets):

I can see how 'args' is plural (even if it is a single string for the
name of a type containing the args), but should it be 'ret' instead of
'rets'?

> @@ -40,34 +40,37 @@ if (%(err)s) {
>  ''',
>                   err=err)
>  
> -def gen_sync_call(name, args, ret_type):
> -    ret = ""
> -    arglist=""
> -    retval=""
> -    if ret_type:
> -        retval = "retval = "
> +def gen_call(name, args, rets):

At least you're consistent on naming it 'rets',

> +    ret = ''

and the naming lets you distinguish between the parameter (the type
describing returned fields) and the local string (the generated C code
holding the return information).

> @@ -82,45 +76,46 @@ def generate_event_implement(api_name, event_name, params):

>  
> +            # Ugly: need to cast away the const
>              if memb.type.name == "str":
> -                var_type = "(char **)"
> +                cast = "(char **)"

And to think I called it out in a previous patch. So you noticed it too :)

Don't you want to use '(char **)' here, since it is a literal string
destined for generated C?

>              else:
> -                var_type = ""
> +                cast = ""

and '' here?

> +++ b/scripts/qapi-types.py
> @@ -16,22 +16,22 @@ from qapi import *
>  def gen_fwd_object_or_array(name):
>      return mcgen('''
>  
> -typedef struct %(name)s %(name)s;
> +typedef struct %(c_name)s %(c_name)s;
>  ''',
> -                 name=c_name(name))
> +                 c_name=c_name(name))
>  
>  def gen_array(name, element_type):
>      return mcgen('''
>  
> -struct %(name)s {
> +struct %(c_name)s {
>      union {
>          %(c_type)s value;
>          uint64_t padding;
>      };
> -    struct %(name)s *next;
> +    struct %(c_name)s *next;

May be some churn here if you like my comment earlier in the series that
this 'struct' is redundant.

> +++ b/scripts/qapi-visit.py

> -def generate_visit_struct_fields(name, members, base = None):
> +def gen_visit_struct_fields(name, base, members):
>      struct_fields_seen.add(name)

> -                     type=base.c_name(), c_name=c_name('base'))
> +                     c_type=base.c_name(), c_name=c_name('base'))

Possible churn here based on my earlier comments about c_name(constant)
being constant.

Fairly big, but aside from some minor '' quoting issues, I didn't see
anything wrong.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]