Re: [Qemu-devel] [PATCH RFC v2 06/47] qapi: Drop unused and useless parameters and variables

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

I'll rephrase the commit message to make this more obvious.

>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  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.

It'll go away in PATCH 27.  Let's keep this patch simple.

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

Likewise.

> 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 <address@hidden>

Thanks!