Re: [Qemu-devel] [PATCH v11 06/15] qapi: Visit variants in visit_type_FOO_fields()

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> We initially created the static visit_type_FOO_fields() helper
> function for reuse of code - we have cases where the initial
> setup for a visit has different allocation (depending on whether
> the fields represent a stand-alone type or are embedded as part
> of a larger type), but where the actual field visits are
> identical once a pointer is available.
>
> Up until the previous patch, visit_type_FOO_fields() was only
> used for structs (no variants), so it was covering every field
> for each type where it was emitted.
>
> Meanwhile, the code for visiting unions looks like:
>
> static visit_type_U_fields() {
>     visit base;
>     visit local_members;
> }
> visit_type_U() {
>     visit_start_struct();
>     visit_type_U_fields();
>     visit variants;
>     visit_end_struct();
> }
>
> which splits the fields of the union visit across two functions.
> Move the code to visit variants to live inside visit_type_U_fields(),
> while making it conditional on having variants so that all other
> instances of the helper function remain unchanged.  This is also
> a step closer towards unifying struct and union visits, and towards
> allowing one union type to be the branch of another flat union.
>
> The resulting diff to the generated code is a bit hard to read,

Mostly because a few visit_type_T_fields() get generated in a different
place.  If I move them back manually for a diff, the patch's effect
becomes clear enough: the switch to visit the variants and the
visit_start_union() guarding it moves from visit_type_T() to
visit_type_T_fields().  That's what the commit message promises.

> but it can be verified that it touches only union types, and that
> the end result is the following general structure:
>
> static visit_type_U_fields() {
>     visit base;
>     visit local_members;
>     visit variants;
> }
> visit_type_U() {
>     visit_start_struct();
>     visit_type_U_fields();
>     visit_end_struct();
> }
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v11: new patch
> ---
>  scripts/qapi-visit.py | 104 
> +++++++++++++++++++++++++-------------------------
>  1 file changed, 53 insertions(+), 51 deletions(-)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 1051710..6cea7d6 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -71,11 +71,16 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, 
> %(c_type)s **obj, Error *
>      return ret
>
>
> -def gen_visit_struct_fields(name, base, members):
> +def gen_visit_struct_fields(name, base, members, variants=None):

Two callers:

* gen_visit_union(): the new code here comes from there, except it's now
  completely guarded by if variants.  Effect: generated code motion.

* gen_visit_struct(): passes no variants, guard false, thus no change.

I think the patch becomes slightly clearer if you make the variants
parameter mandatory.  It'll probably become mandatory anyway when we
unify gen_visit_struct() and gen_visit_union().

>      ret = ''
>
>      if base:
>          ret += gen_visit_fields_decl(base)
> +    if variants:
> +        for var in variants.variants:
> +            # Ugly special case for simple union TODO get rid of it
> +            if not var.simple_union_type():
> +                ret += gen_visit_implicit_struct(var.type)
>
>      struct_fields_seen.add(name)
>      ret += mcgen('''
> @@ -96,8 +101,49 @@ static void visit_type_%(c_name)s_fields(Visitor *v, 
> %(c_name)s **obj, Error **e
>
>      ret += gen_visit_fields(members, prefix='(*obj)->')
>
> -    # 'goto out' produced for base, and by gen_visit_fields() for each member
> -    if base or members:
> +    if variants:
> +        ret += mcgen('''
> +    if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
> +        goto out;
> +    }
> +    switch ((*obj)->%(c_name)s) {
> +''',
> +                     c_name=c_name(variants.tag_member.name))
> +
> +        for var in variants.variants:
> +            # TODO ugly special case for simple union
> +            simple_union_type = var.simple_union_type()
> +            ret += mcgen('''
> +    case %(case)s:
> +''',
> +                         case=c_enum_const(variants.tag_member.type.name,
> +                                           var.name,
> +                                           variants.tag_member.type.prefix))
> +            if simple_union_type:
> +                ret += mcgen('''
> +        visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err);
> +''',
> +                             c_type=simple_union_type.c_name(),
> +                             c_name=c_name(var.name))
> +            else:
> +                ret += mcgen('''
> +        visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
> +''',
> +                             c_type=var.type.c_name(),
> +                             c_name=c_name(var.name))
> +            ret += mcgen('''
> +        break;
> +''')
> +
> +        ret += mcgen('''
> +    default:
> +        abort();
> +    }
> +''')
> +
> +    # 'goto out' produced for base, by gen_visit_fields() for each member,
> +    # and if variants were present
> +    if base or members or variants:
>          ret += mcgen('''
>
>  out:
> @@ -239,12 +285,7 @@ out:
>
>
>  def gen_visit_union(name, base, members, variants):
> -    ret = gen_visit_struct_fields(name, base, members)
> -
> -    for var in variants.variants:
> -        # Ugly special case for simple union TODO get rid of it
> -        if not var.simple_union_type():
> -            ret += gen_visit_implicit_struct(var.type)
> +    ret = gen_visit_struct_fields(name, base, members, variants)
>
>      ret += mcgen('''
>
> @@ -260,54 +301,15 @@ void visit_type_%(c_name)s(Visitor *v, const char 
> *name, %(c_name)s **obj, Error
>          goto out_obj;
>      }
>      visit_type_%(c_name)s_fields(v, obj, &err);
> -''',
> -                 c_name=c_name(name))
> -    ret += gen_err_check(label='out_obj')
> -    ret += mcgen('''
> -    if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
> -        goto out_obj;
> -    }
> -    switch ((*obj)->%(c_name)s) {
> -''',
> -                 c_name=c_name(variants.tag_member.name))
> -
> -    for var in variants.variants:
> -        # TODO ugly special case for simple union
> -        simple_union_type = var.simple_union_type()
> -        ret += mcgen('''
> -    case %(case)s:
> -''',
> -                     case=c_enum_const(variants.tag_member.type.name,
> -                                       var.name,
> -                                       variants.tag_member.type.prefix))
> -        if simple_union_type:
> -            ret += mcgen('''
> -        visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err);
> -''',
> -                         c_type=simple_union_type.c_name(),
> -                         c_name=c_name(var.name))
> -        else:
> -            ret += mcgen('''
> -        visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
> -''',
> -                         c_type=var.type.c_name(),
> -                         c_name=c_name(var.name))
> -        ret += mcgen('''
> -        break;
> -''')
> -
> -    ret += mcgen('''
> -    default:
> -        abort();
> -    }
> -out_obj:
>      error_propagate(errp, err);
>      err = NULL;
> +out_obj:
>      visit_end_struct(v, &err);
>  out:
>      error_propagate(errp, err);
>  }
> -''')
> +''',
> +                 c_name=c_name(name))
>
>      return ret