Re: [Qemu-devel] [PATCH v2 13/19] qapi-visit: Simplify visit of empty branch in union

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Now that we have is_empty() and gen_visit_fields_call(), it's

s/fields/members/.  Better grep all the commit messages for "fields".

> fairly easy to skip the visit of a variant type that has no
> members.

I figure that alone would've been just as easy before
gen_visit_members_call(), but see below.

>           Only one such instance exists at the moment
> (CpuInfoOther),

My testing shows qapi-schema-test.json's Empty2 is also affected:

@@ -198,7 +198,6 @@ void visit_type_Empty2_members(Visitor *
 {
     Error *err = NULL;
 
-    visit_type_Empty1_members(v, (Empty1 *)obj, &err);
     if (err) {
         goto out;
     }

This isn't "the visit of a variant type that has no members", it's a
visit of an empty base type.

If you go back to PATCH 10, you can see this patch's stated purpose is
tied to the last hunk there, which uses gen_visit_members_call() for a
variant.  Its other use is for the base.

Suggest to rephrase this patch's commit message to capture both.

>                 but the idea of a union where some branches
> add no fields beyond the base type is common enough that we
> may add syntax for other cases, as in
>   { 'union':'U', 'base':'B', 'discriminator':'D',
>     'data': { 'default': {}, 'extra':'Type' } }

I find 'default' confusing, because a variant record's default case is
the case that applies to all the tag values not explicitly listed.  What
about simply 'case1' and 'case2'?

> Note that the Abort type is another example of an empty type,
> but it is only used by a simple union rather than a flat
> union; and with simple unions, the wrapper type providing
> the 'data' QMP key is not empty.

As so often, simple unions are anything but.

> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v2: no change
> ---
>  scripts/qapi-visit.py | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index dbae00c..a9de393 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -37,7 +37,9 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s 
> *obj, Error **errp);
>  def gen_visit_members_call(typ, c_name):
>      ret = ''
>      assert isinstance(typ, QAPISchemaObjectType)
> -    if typ.is_implicit():
> +    if typ.is_empty():
> +        pass
> +    elif typ.is_implicit():
>          # TODO ugly special case for simple union
>          assert len(typ.members) == 1
>          assert not typ.variants

Okay because the patch is trivial.  If it wasn't, I'd say avoiding the
call isn't worthwhile; the compiler might even optimize it to nothing.