On 07/01/2015 02:21 PM, Markus Armbruster wrote:
> gen_sync_call()'s parameter indent is useless: gen_sync_call() uses it
> only as optional argument for push_indent() and pop_indent(), their
> default is four, and gen_sync_call()'s only caller passes four.
> 
> gen_visitor_input_containers_decl()'s parameter obj is always
> "QOBJECT(args)".

It might be nice to call out that several other functions are also
stripped of unused arguments.  I was assuming that only two functions
(and their callers) would be modified, but the patch touched more:

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-commands.py | 27 +++++++++++++--------------
>  scripts/qapi-event.py    |  1 -
>  scripts/qapi-types.py    |  1 -
>  scripts/qapi-visit.py    | 47 ++++++++++++++++++++++-------------------------
>  scripts/qapi.py          |  1 -
>  5 files changed, 35 insertions(+), 42 deletions(-)

> @@ -161,7 +160,7 @@ qapi_dealloc_visitor_cleanup(md);
>      pop_indent()
>      return ret.rstrip()
>  
> -def gen_marshal_output(name, args, ret_type, middle_mode):
> +def gen_marshal_output(name, ret_type):
>      if not ret_type:
>          return ""

For example, gen_marshal_output() was not mentioned in the commit message.

>  
> @@ -194,14 +193,14 @@ out:
>  
>      return ret
>  
> -def gen_marshal_input_decl(name, args, ret_type, middle_mode):
> +def gen_marshal_input_decl(name, middle_mode):

Or gen_marshal_input_decl()

> +++ b/scripts/qapi-event.py
> @@ -199,7 +199,6 @@ const char *%(event_enum_name)s_lookup[] = {
>  ''',
>                  event_enum_name = event_enum_name)
>  
> -    i = 0
>      for string in event_enum_strings:

I guess the subject line covered deletion of unused internal variables.

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

> @@ -441,44 +440,42 @@ fdecl.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DECL"))

>      elif expr.has_key('union'):
>          ret = generate_visit_union(expr)
> -        ret += generate_visit_list(expr['union'], expr['data'])
> +        ret += generate_visit_list(expr['union'])
>          fdef.write(ret)
>  
>          enum_define = discriminator_find_enum_define(expr)
>          ret = ""
>          if not enum_define:
> -            ret = generate_decl_enum('%sKind' % expr['union'],
> -                                     expr['data'].keys())
> -        ret += generate_declaration(expr['union'], expr['data'])
> +            ret = generate_decl_enum('%sKind' % expr['union'])
> +        ret += generate_declaration(expr['union'])

As long as you are touching this, generate_decl_enum(expr['union'] +
'Kind') would read nicer.

>          fdecl.write(ret)
>      elif expr.has_key('alternate'):
>          ret = generate_visit_alternate(expr['alternate'], expr['data'])
> -        ret += generate_visit_list(expr['alternate'], expr['data'])
> +        ret += generate_visit_list(expr['alternate'])
>          fdef.write(ret)
>  
> -        ret = generate_decl_enum('%sKind' % expr['alternate'],
> -                                 expr['data'].keys())
> -        ret += generate_declaration(expr['alternate'], expr['data'])
> +        ret = generate_decl_enum('%sKind' % expr['alternate'])

Same here.

At any rate, no change to generated output.  So assuming you are happy
with the commit message, or take my advice to improve it, the code
cleanup itself is fine.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org