Re: [Qemu-devel] [PATCH v9 06/17] qapi-visit: Remove redundant functions for flat union base

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> The code for visiting the base class of a child struct created
> visit_type_Base_fields() which covers all fields of Base; while
> the code for visiting the base class of a flat union created
> visit_type_Union_fields() covering all fields of the base
> except the discriminator.  But if the base class were to always
> visit all its fields, then we wouldn't need a separate visit of
> the discriminator for a union.  Not only is consistently
> visiting all fields easier to understand, it lets us share code.
>
> Now that gen_visit_struct_fields() can potentially collect more
> than one function into 'ret', a regular expression searching for
> whether a label was used may hit a false positive within the
> body of the first function.  But using a regex was overkill,
> since we can easily determine when we jumped to a label.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v9: (no v6-8): hoist from v5 35/46; rebase to master; fix indentation
> botch in gen_visit_union(); polish commit message
> ---
>  scripts/qapi-visit.py | 35 +++++++++++++++++------------------
>  1 file changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 8aae8da..91bf350 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -90,7 +90,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, 
> %(c_name)s **obj, Error **e
>
>      ret += gen_visit_fields(members, prefix='(*obj)->')
>
> -    if re.search('^ *goto out;', ret, re.MULTILINE):
> +    if base or members:

What if we have an empty base and no members?  Empty base is a
pathological case, admittedly.

>          ret += mcgen('''
>
>  out:
> @@ -221,8 +221,8 @@ def gen_visit_union(name, base, variants):
>      ret = ''
>
>      if base:
> -        members = [m for m in base.members if m != variants.tag_member]
> -        ret += gen_visit_struct_fields(name, None, members)
> +        ret += gen_visit_struct_fields(base.name, base.base,
> +                                       base.local_members)
>
>      for var in variants.variants:
>          # Ugly special case for simple union TODO get rid of it
> @@ -247,31 +247,30 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s 
> **obj, const char *name, Error
>
>      if base:
>          ret += mcgen('''
> -    visit_type_%(c_name)s_fields(v, obj, &err);
> +    visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err);
>  ''',
> -                     c_name=c_name(name))
> -        ret += gen_err_check(label='out_obj')
> -
> -    tag_key = variants.tag_member.name
> -    if not variants.tag_name:
> -        # we pointlessly use a different key for simple unions
> -        tag_key = 'type'
> -    ret += mcgen('''
> +                     c_name=base.c_name())
> +    else:
> +        ret += mcgen('''
>      visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
> -    if (err) {
> -        goto out_obj;
> -    }
> +''',
> +                     c_type=variants.tag_member.type.c_name(),
> +                     # TODO ugly special case for simple union
> +                     # Use same tag name in C as on the wire to get rid of
> +                     # it, then: c_name=c_name(variants.tag_member.name)
> +                     c_name='kind',
> +                     name=variants.tag_member.name)
> +    ret += gen_err_check(label='out_obj')
> +    ret += mcgen('''
>      if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
>          goto out_obj;
>      }
>      switch ((*obj)->%(c_name)s) {
>  ''',
> -                 c_type=variants.tag_member.type.c_name(),
>                   # TODO ugly special case for simple union
>                   # Use same tag name in C as on the wire to get rid of
>                   # it, then: c_name=c_name(variants.tag_member.name)
> -                 c_name=c_name(variants.tag_name or 'kind'),
> -                 name=tag_key)
> +                 c_name=c_name(variants.tag_name or 'kind'))
>
>      for var in variants.variants:
>          # TODO ugly special case for simple union

Diff is confusing (not your fault).  Let me compare code before and
after real slow.

= Before =

  def gen_visit_union(name, base, variants):
      ret = ''

0. base is None if and only if the union is simple.

1. If it's a flat union, generate its visit_type_NAME_fields().  This
function visits the union's non-variant members *except* the
discriminator.  Since a simple union has no non-variant members other
than the discriminator, generate it only for flat unions.

      if base:
          members = [m for m in base.members if m != variants.tag_member]
          ret += gen_visit_struct_fields(name, None, members)

2. Generate the visit_type_implicit_FOO() we're going to need.

      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)

3. Generate its visit_type_NAME().

      ret += mcgen('''

  void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, 
Error **errp)
  {
      Error *err = NULL;

      visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), 
&err);
      if (err) {
          goto out;
      }
      if (!*obj) {
          goto out_obj;
      }
  ''',
                   c_name=c_name(name), name=name)

3.a. If it's a flat union, generate the call of
visit_type_NAME_fields().  Not necessary for simple unions, see 1.

      if base:
          ret += mcgen('''
      visit_type_%(c_name)s_fields(v, obj, &err);
  ''',
                       c_name=c_name(name))
          ret += gen_err_check(label='out_obj')

3.b. Generate visit of discriminator.

      tag_key = variants.tag_member.name
      if not variants.tag_name:
          # we pointlessly use a different key for simple unions
          tag_key = 'type'
      ret += mcgen('''
      visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
      if (err) {
          goto out_obj;
      }

3.c. Generate visit of the active variant.

      if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
          goto out_obj;
      }
      switch ((*obj)->%(c_name)s) {
  ''',
                   c_type=variants.tag_member.type.c_name(),
                   # TODO ugly special case for simple union
                   # Use same tag name in C as on the wire to get rid of
                   # it, then: c_name=c_name(variants.tag_member.name)
                   c_name=c_name(variants.tag_name or 'kind'),
                   name=tag_key)

[Some stuff the patch doesn't change omitted...]

  out_obj:
      error_propagate(errp, err);
      err = NULL;
      if (*obj) {
          visit_end_union(v, !!(*obj)->data, &err);
      }
      error_propagate(errp, err);
      err = NULL;
      visit_end_struct(v, &err);
  out:
      error_propagate(errp, err);
  }
  ''')

      return ret

= After =

  def gen_visit_union(name, base, variants):
      ret = ''

0. base is None if and only if the union is simple.

1. If it's a flat union, generate its visit_type_NAME_fields().  This
function visits the union's non-variant members *including* the
discriminator.  However, we generate it only for flat unions.  Simple
unions have no non-variant members other than the discriminator.

      if base:
          ret += gen_visit_struct_fields(base.name, base.base,
                                         base.local_members)

2. Generate the visit_type_implicit_FOO() we're going to need.

      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)

3. Generate its visit_type_NAME().

      ret += mcgen('''

  void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, 
Error **errp)
  {
      Error *err = NULL;

      visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), 
&err);
      if (err) {
          goto out;
      }
      if (!*obj) {
          goto out_obj;
      }
  ''',
                   c_name=c_name(name), name=name)

3.a. If it's a flat union, generate the call of
visit_type_NAME_fields().  Not done for simple unions, see 1.

      if base:
          ret += mcgen('''
      visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err);
  ''',
                       c_name=base.c_name())

3.b. If it's a simple union, generate the visit of the sole non-variant
member inline.

      else:
          ret += mcgen('''
      visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
  ''',
                       c_type=variants.tag_member.type.c_name(),
                       # TODO ugly special case for simple union
                       # Use same tag name in C as on the wire to get rid of
                       # it, then: c_name=c_name(variants.tag_member.name)
                       c_name='kind',
                       name=variants.tag_member.name)

3.a+b. Generate the error check for visit of non-variant members

      ret += gen_err_check(label='out_obj')

3.c. Generate visit of the active variant.

      ret += mcgen('''
      if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
          goto out_obj;
      }
      switch ((*obj)->%(c_name)s) {
  ''',
                   # TODO ugly special case for simple union
                   # Use same tag name in C as on the wire to get rid of
                   # it, then: c_name=c_name(variants.tag_member.name)
                   c_name=c_name(variants.tag_name or 'kind'))

[Some stuff the patch doesn't change omitted...]

  out_obj:
      error_propagate(errp, err);
      err = NULL;
      if (*obj) {
          visit_end_union(v, !!(*obj)->data, &err);
      }
      error_propagate(errp, err);
      err = NULL;
      visit_end_struct(v, &err);
  out:
      error_propagate(errp, err);
  }
  ''')

      return ret

Okay, the change to gen_visit_union() looks sane.

Can we go one step further and generate and use visit_type_NAME_fields()
even for simple unions?