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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

I'm afraid splitting would increase churn without much gain.  If you
want me to split, I can try.

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

I now realize readers may find this odd.

The QMP specification talks about command arguments and command
responses.

The QMP wire format uses 'arguments' and 'return'.

The schema language uses 'data' and 'returns'.  Near-perfect score on
terminology inconsistency, as usual.  Anyway, 'data' is plural (and a
rather unhelpful choice of syntax).  'returns' could either be the
plural of the noun "return", or the third person singular of the verb
"to return".

Permit me a philosophical digression.  The common way to do functions in
programming is to have multiple arguments and a single return value.  I
believe this is mostly common machines' calling conventions leaking into
languages.  From a more abstract point of view, there's no structural
difference between function arguments and values: both are simply an
element of an arbitrary set (domain and codomain, respectively).  In
particular, both can be tuples.

It's perfectly sane to have functions take exactly one argument and
yield exactly one value.  Some functional languages work that way.

But when both argument and value are generally tuples anyway, as they
are in QAPI/QMP, it's more natural to talk about arguments and return
values.  I abbreviated to args and rets.  There's method to my madness
;)

I'm open to better ideas on terminology.

>> @@ -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?

Yes.

>>              else:
>> -                var_type = ""
>> +                cast = ""
>
> and '' here?

Yes.

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

Can drop it in this patch.

I'm not into typedef'ing struct/union away unless the type is opaque,
but it's the QEMU style, so let's stick to it.

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

I'm leaning towards leaving it as is just to keep the code similar to
other places generating visit_type_FOO() calls.

> Fairly big, but aside from some minor '' quoting issues, I didn't see
> anything wrong.
>
> Reviewed-by: Eric Blake <address@hidden>

Thanks!