Re: [Qemu-devel] [PATCH V7 06/11] qapi script: use same function to generate enum string

[Markus Armbruster]

Wenchao Xia <address@hidden> writes:

> Prior to this patch, qapi-visit.py used custom code to generate enum
> names used for handling a qapi union. Fix it to instead reuse common
> code, with identical generated results, and allowing future updates to
> generation to only need to touch one place.
>
> Signed-off-by: Wenchao Xia <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  scripts/qapi-types.py |    6 +++---
>  scripts/qapi-visit.py |   21 +++++++++++++++------
>  scripts/qapi.py       |   15 +++++++++++----
>  3 files changed, 29 insertions(+), 13 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 35ad993..656a9a0 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -144,11 +144,11 @@ typedef enum %(name)s
>  
>      i = 0
>      for value in enum_values:
> +        enum_full_value_string = generate_enum_full_value_string(name, value)
>          enum_decl += mcgen('''
> -    %(abbrev)s_%(value)s = %(i)d,
> +    %(enum_full_value_string)s = %(i)d,
>  ''',
> -                     abbrev=de_camel_case(name).upper(),
> -                     value=generate_enum_name(value),
> +                     enum_full_value_string=enum_full_value_string,
>                       i=i)
>          i += 1
>  
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index c6de9ae..87e6df7 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -214,18 +214,23 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, 
> const char *name, Error **
>  ''',
>      name=name)
>  
> +    # For anon union, always use the default enum type automatically 
> generated
> +    # as "'%sKind' % (name)"
> +    discriminator_type_name = '%sKind' % (name)
> +
>      for key in members:
>          assert (members[key] in builtin_types
>              or find_struct(members[key])
>              or find_union(members[key])), "Invalid anonymous union member"
>  
> +        enum_full_value_string = \
> +                  generate_enum_full_value_string(discriminator_type_name, 
> key)
>          ret += mcgen('''
> -        case %(abbrev)s_KIND_%(enum)s:
> +        case %(enum_full_value_string)s:
>              visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err);
>              break;
>  ''',
> -                abbrev = de_camel_case(name).upper(),
> -                enum = c_fun(de_camel_case(key),False).upper(),
> +                enum_full_value_string = enum_full_value_string,
>                  c_type = type_name(members[key]),
>                  c_name = c_fun(key))
>  
> @@ -255,7 +260,10 @@ def generate_visit_union(expr):
>          assert not base
>          return generate_visit_anon_union(name, members)
>  
> +    # There will always be a discriminator in the C switch code, by default 
> it
> +    # is an enum type generated silently as "'%sKind' % (name)"
>      ret = generate_visit_enum('%sKind' % name, members.keys())
> +    discriminator_type_name = '%sKind' % (name)
>  
>      if base:
>          base_fields = find_struct(base)['data']
> @@ -313,13 +321,14 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, 
> const char *name, Error **
>                      visit_end_implicit_struct(m, &err);
>                  }'''
>  
> +        enum_full_value_string = \
> +                  generate_enum_full_value_string(discriminator_type_name, 
> key)

Long line due to very long identifiers.  disc_type_name or disc_type
would do, I think.  Also consider value instead of value_string.

>          ret += mcgen('''
> -            case %(abbrev)s_KIND_%(enum)s:
> +            case %(enum_full_value_string)s:
>                  ''' + fmt + '''
>                  break;
>  ''',
> -                abbrev = de_camel_case(name).upper(),
> -                enum = c_fun(de_camel_case(key),False).upper(),
> +                enum_full_value_string = enum_full_value_string,
>                  c_type=type_name(members[key]),
>                  c_name=c_fun(key))
>  
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index c334ad3..130dced 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -468,12 +468,19 @@ def guardend(name):
>  ''',
>                   name=guardname(name))
>  
> -def generate_enum_name(name):
> -    if name.isupper():
> -        return c_fun(name, False)
> +def _generate_enum_value_string(value):
> +    if value.isupper():
> +        return c_fun(value, False)
>      new_name = ''
> -    for c in c_fun(name, False):
> +    for c in c_fun(value, False):
>          if c.isupper():
>              new_name += '_'
>          new_name += c
>      return new_name.lstrip('_').upper()
> +
> +def generate_enum_full_value_string(enum_name, enum_value):
> +    # generate abbrev string
> +    abbrev_string = de_camel_case(enum_name).upper()
> +    # generate value string
> +    value_string = _generate_enum_value_string(enum_value)
> +    return "%s_%s" % (abbrev_string, value_string)

These comments feel quite superfluous.