Re: [Qemu-devel] [PATCH v6 28/36] qapi: Prefer 'struct' over 'type' in generator

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Referring to "type" as both a meta-type (built-in, enum, union,
> alternte, or struct) and a specific type (the name that the
> schema uses for declaring structs) is confusing.  The confusion
> is only made worse by the fact that the generator mostly already
> refers to struct even when dealing with expr['type'].  This
> commit changes the generator to consistently refer to it as
> struct everywhere, plus a single back-compat tweak that allows
> accepting the existing .json files as-is, so that the meat of
> this change is separate from the mindless churn of that change.
>
> Fix the testsuite fallout for error messages that change, and
> in some cases, become more legible.

Some become temporarily confusing: message refers to 'struct', while
schema still has 'type'.  I don't mind.

>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
>
> v6: new patch
> ---
>  scripts/qapi-types.py                              | 16 ++++----
>  scripts/qapi-visit.py                              |  8 ++--
>  scripts/qapi.py                                    | 42 +++++++++++++--------
>  tests/qapi-schema/alternate-good.out               |  4 +-
>  tests/qapi-schema/bad-ident.err                    |  2 +-
>  tests/qapi-schema/bad-type-bool.err                |  2 +-
>  tests/qapi-schema/data-member-array.out            |  4 +-
>  tests/qapi-schema/double-type.err                  |  2 +-
>  tests/qapi-schema/flat-union-base-star.err         |  2 +-
>  tests/qapi-schema/flat-union-base-union.err        |  2 +-
>  tests/qapi-schema/flat-union-branch-clash.out      | 12 +++---
>  .../flat-union-invalid-discriminator.err           |  2 +-
>  tests/qapi-schema/flat-union-reverse-define.out    | 12 +++---
>  tests/qapi-schema/qapi-schema-test.out             | 44 
> +++++++++++-----------
>  tests/qapi-schema/union-invalid-base.err           |  2 +-
>  tests/qapi-schema/unknown-expr-key.err             |  2 +-
>  16 files changed, 85 insertions(+), 73 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 9c8d68c..a429d9e 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -83,7 +83,7 @@ def generate_struct_fields(members):
>
>  def generate_struct(expr):
>
> -    structname = expr.get('type', "")
> +    structname = expr.get('struct', "")
>      fieldname = expr.get('field', "")
>      members = expr['data']
>      base = expr.get('base')
> @@ -394,8 +394,8 @@ fdecl.write(guardend("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
>
>  for expr in exprs:
>      ret = "\n"
> -    if expr.has_key('type'):
> -        ret += generate_fwd_struct(expr['type'], expr['data'])
> +    if expr.has_key('struct'):
> +        ret += generate_fwd_struct(expr['struct'], expr['data'])
>      elif expr.has_key('enum'):
>          ret += generate_enum(expr['enum'], expr['data']) + "\n"
>          ret += generate_fwd_enum_struct(expr['enum'], expr['data'])
> @@ -435,12 +435,12 @@ if do_builtins:
>
>  for expr in exprs:
>      ret = "\n"
> -    if expr.has_key('type'):
> +    if expr.has_key('struct'):
>          ret += generate_struct(expr) + "\n"
> -        ret += generate_type_cleanup_decl(expr['type'] + "List")
> -        fdef.write(generate_type_cleanup(expr['type'] + "List") + "\n")
> -        ret += generate_type_cleanup_decl(expr['type'])
> -        fdef.write(generate_type_cleanup(expr['type']) + "\n")
> +        ret += generate_type_cleanup_decl(expr['struct'] + "List")
> +        fdef.write(generate_type_cleanup(expr['struct'] + "List") + "\n")
> +        ret += generate_type_cleanup_decl(expr['struct'])
> +        fdef.write(generate_type_cleanup(expr['struct']) + "\n")
>      elif expr.has_key('union'):
>          ret += generate_union(expr, 'union')
>          ret += generate_type_cleanup_decl(expr['union'] + "List")
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 3e11089..eeeca82 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -178,7 +178,7 @@ def generate_visit_struct_body(field_prefix, name, 
> members):
>
>  def generate_visit_struct(expr):
>
> -    name = expr['type']
> +    name = expr['struct']
>      members = expr['data']
>      base = expr.get('base')
>
> @@ -549,12 +549,12 @@ if do_builtins:
>          fdef.write(generate_visit_list(typename, None))
>
>  for expr in exprs:
> -    if expr.has_key('type'):
> +    if expr.has_key('struct'):
>          ret = generate_visit_struct(expr)
> -        ret += generate_visit_list(expr['type'], expr['data'])
> +        ret += generate_visit_list(expr['struct'], expr['data'])
>          fdef.write(ret)
>
> -        ret = generate_declaration(expr['type'], expr['data'])
> +        ret = generate_declaration(expr['struct'], expr['data'])
>          fdecl.write(ret)
>      elif expr.has_key('union'):
>          ret = generate_visit_union(expr)
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 8c83de0..c246570 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -419,7 +419,7 @@ def check_union(expr, expr_info):
>      members = expr['data']
>      values = { 'MAX': '(automatic)' }
>
> -    # If the object has a member 'base', its value must name a complex type,
> +    # If the object has a member 'base', its value must name a struct,
>      # and there must be a discriminator.
>      if base is not None:
>          if discriminator is None:
> @@ -448,18 +448,18 @@ def check_union(expr, expr_info):
>          base_fields = find_base_fields(base)
>          if not base_fields:
>              raise QAPIExprError(expr_info,
> -                                "Base '%s' is not a valid type"
> +                                "Base '%s' is not a valid struct"
>                                  % base)
>
>          # The value of member 'discriminator' must name a non-optional
> -        # member of the base type.
> +        # member of the base struct.
>          check_name(expr_info, "Discriminator of flat union '%s'" % name,
>                     discriminator)
>          discriminator_type = base_fields.get(discriminator)
>          if not discriminator_type:
>              raise QAPIExprError(expr_info,
>                                  "Discriminator '%s' is not a member of base "
> -                                "type '%s'"
> +                                "struct '%s'"
>                                  % (discriminator, base))
>          enum_define = find_enum(discriminator_type)
>          allow_metas=['struct']
> @@ -546,7 +546,7 @@ def check_enum(expr, expr_info):
>          values[key] = member
>
>  def check_struct(expr, expr_info):
> -    name = expr['type']
> +    name = expr['struct']
>      members = expr['data']
>
>      check_type(expr_info, "'data' for type '%s'" % name, members,
                  allow_dict=True, allow_optional=True)
       check_type(expr_info, "'base' for type '%s'" % name, expr.get('base'),
                  allow_metas=['struct'])

Want to say "for struct '%s'" here?

> @@ -565,7 +565,7 @@ def check_exprs(schema):
>              check_union(expr, info)
>          elif expr.has_key('alternate'):
>              check_alternate(expr, info)
> -        elif expr.has_key('type'):
> +        elif expr.has_key('struct'):
>              check_struct(expr, info)
>          elif expr.has_key('command'):
>              check_command(expr, info)
> @@ -617,6 +617,20 @@ def parse_schema(input_file):
>          for expr_elem in schema.exprs:
>              expr = expr_elem['expr']
>              info = expr_elem['info']
> +
> +            # back-compat hack until all schemas have been converted;
> +            # preserve the ordering of the original expression
> +            if expr.has_key('type'):
> +                seen_type = False
> +                for (key, value) in expr.items():
> +                    if key == 'type':
> +                        seen_type = True
> +                        del expr['type']
> +                        expr['struct'] = value
> +                    elif seen_type:
> +                        del expr[key]
> +                        expr[key] = value
> +
>              if expr.has_key('enum'):
>                  check_keys(expr_elem, 'enum', ['data'])
>                  add_enum(expr['enum'], info, expr['data'])
> @@ -627,8 +641,8 @@ def parse_schema(input_file):
>              elif expr.has_key('alternate'):
>                  check_keys(expr_elem, 'alternate', ['data'])
>                  add_name(expr['alternate'], info, 'alternate')
> -            elif expr.has_key('type'):
> -                check_keys(expr_elem, 'type', ['data'], ['base'])
> +            elif expr.has_key('struct'):
> +                check_keys(expr_elem, 'struct', ['data'], ['base'])
>                  add_struct(expr, info)
>              elif expr.has_key('command'):
>                  check_keys(expr_elem, 'command', [],
> @@ -745,11 +759,9 @@ def type_name(name):
>          return c_list_type(name[0])
>      return name
>
> -def add_name(name, info, meta, implicit = False, source = None):
> +def add_name(name, info, meta, implicit = False):
>      global all_names
> -    if not source:
> -        source = "'%s'" % meta
> -    check_name(info, source, name)
> +    check_name(info, "'%s'" % meta, name)
>      if name in all_names:
>          raise QAPIExprError(info,
>                              "%s '%s' is already defined"
> @@ -762,14 +774,14 @@ def add_name(name, info, meta, implicit = False, source 
> = None):
>
>  def add_struct(definition, info):
>      global struct_types
> -    name = definition['type']
> -    add_name(name, info, 'struct', source="'type'")
> +    name = definition['struct']
> +    add_name(name, info, 'struct')
>      struct_types.append(definition)
>
>  def find_struct(name):
>      global struct_types
>      for struct in struct_types:
> -        if struct['type'] == name:
> +        if struct['struct'] == name:
>              return struct
>      return None
>
[...]

Reviewed-by: Markus Armbruster <address@hidden>