Re: [Qemu-devel] [PATCH v10 07/25] qapi-visit: Split off visit_type_FOO_fields forward decl

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> We generate a static visit_type_FOO_fields() for every type
> FOO.  However, sometimes we need a forward declaration. Split
> the code to generate the forward declaration out of
> gen_visit_implicit_struct() into a new gen_visit_fields_decl(),
> and also prepare for a forward declaration to be emitted
> during gen_visit_struct(), so that a future patch can switch
> from using visit_type_FOO_implicit() to the simpler
> visit_type_FOO_fields() as part of unboxing the base class
> of a struct.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v10: new patch, split from 5/17
> ---
>  scripts/qapi-visit.py | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index e878018..7204ed0 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -15,7 +15,12 @@
>  from qapi import *
>  import re
>
> +# visit_type_FOO_implicit() is emitted as needed; track if it has already
> +# been output. No forward declaration is needed.
>  implicit_structs_seen = set()

I initially read this as "No forward is needed then", but that's wrong.
Suggest to drop that sentence.

> +
> +# visit_type_FOO_fields() is always emitted; track if a forward declaration
> +# or implementation has already been output.
>  struct_fields_seen = set()

Yup.

> @@ -29,19 +34,24 @@ void visit_type_%(c_name)s(Visitor *v, %(c_type)sobj, 
> const char *name, Error **
>                   c_name=c_name(name), c_type=c_type)
>
>
> +def gen_visit_fields_decl(typ):
> +    ret = ''
> +    if typ.name not in struct_fields_seen:
> +        ret += mcgen('''
> +
> +static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error 
> **errp);
> +''',
> +                     c_type=typ.c_name())
> +        struct_fields_seen.add(typ.name)
> +    return ret
> +
> +
>  def gen_visit_implicit_struct(typ):
>      if typ in implicit_structs_seen:
>          return ''
>      implicit_structs_seen.add(typ)
>
> -    ret = ''
> -    if typ.name not in struct_fields_seen:
> -        # Need a forward declaration
> -        ret += mcgen('''
> -
> -static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error 
> **errp);
> -''',
> -                     c_type=typ.c_name())
> +    ret = gen_visit_fields_decl(typ)
>
>      ret += mcgen('''
>
> @@ -62,13 +72,12 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, 
> %(c_type)s **obj, Error *
>
>
>  def gen_visit_struct_fields(name, base, members):
> -    struct_fields_seen.add(name)
> -
>      ret = ''
>
>      if base:
>          ret += gen_visit_implicit_struct(base)
>
> +    struct_fields_seen.add(name)
>      ret += mcgen('''
>

Minor cleanup not mentioned in commit message.  Okay.

>  static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error 
> **errp)
> @@ -100,7 +109,11 @@ out:
>
>
>  def gen_visit_struct(name, base, members):
> -    ret = gen_visit_struct_fields(name, base, members)
> +    ret = ''
> +    if base:
> +        ret += gen_visit_fields_decl(base)
> +
> +    ret += gen_visit_struct_fields(name, base, members)
>
>      # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
>      # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj

What's the purpose of this hunk?