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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6834 bytes --]

On 10/21/2015 11:36 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
>>

>> +++ 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.

I'm going to filter the re.search cleanups into its own prereq patch.
But yes, it will need to care for empty base and no members (hmm, I
really ought to add positive tests to qapi-schema-test for an empty
inherited struct, to make sure I'm getting it right - even if we don't
want that patch in the final series).


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

I also plan for v10 to include a diff of the generated code in the
commit message, if that will help make the change more obvious.

> 
> = 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

where NAME is the union name.

> 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)

Could be made slightly simpler by generating these while we iterate over
cases (but we'd have to be careful to generate into multiple variables,
and then concat together at the end, since we can't generate one
function in the body of the other).

> 
> 3. Generate its visit_type_NAME().
> 

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

Again, important to note that this was visit_type_UNION_fields().

> 3.b. Generate visit of discriminator.
> 

> 
> 3.c. Generate visit of the active variant.
> 

> = 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)

Note that this creates visit_type_BASE_fields() (a different function).

> 
> 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().
> 

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

Again, now NAME is the base name rather than the union name.  Subtle but
important difference!

> 
>       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.
> 

> 
> 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.
> 

> 
> Okay, the change to gen_visit_union() looks sane.

Yes, you got it all correct.

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

Not easily.  Remember, for flat unions, we are calling
visit_type_BASE_fields, not visit_type_UNION_fields.  There is no base
for a simple union.

What I _am_ planning for future patches is the following:

Change QAPISchemaObjectType for simple unions and alternates to set
.local_members to the one-element implicit discriminator (right now, it
is always an empty array, and we even assert that bool(.local_members)
and bool(.variants) are mutually-exclusive in at least qapi-types.py
visit_object_type()).  Flat unions would keep .local_members as an empty
array (there is no local member, just the inherited members from the
base class, which includes the inherited discriminator).

Then merge gen_visit_struct() and gen_visit_union() to look like:

if base:
    # includes discriminator for flat union
    call visit_type_BASE_fields()
for m in .local_members
    # includes discriminator for simple union
    call visit_type_MEMBER()
if variants
    emit switch statement to visit each branch

But if we want, that 'for m in .local_members' block can indeed be
implemented via a call to visit_type_UNION_fields(), if that is any more
efficient to implement.  I guess it also boils down to deciding if
visit_type_FOO_fields() should continue to be unconditional (either for
all types, or at least for all types with non-empty .local_members).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]