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 <arm...@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 <ebl...@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
