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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

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

Don't take my reluctance to take some positive tests as general
opposition towards positive tests!  Positive tests for corner cases like
"empty base" are valuable.

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

I doubt it would be an improvement.  The loop is trivial, so
de-duplicating it doesn't have much value.  Having generator code
arranged in the same order as the generated code *does* have value.

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

Missed this detail, thanks.  The old visit_type_UNION_fields() is
visit_type_BASE_fields() less the tag visit.  Reusing
visit_type_BASE_fields() instead behaves as I described above, so my
analysis remains valid regardless.

visit_type_BASE_fields() should be generated when gen_visit_struct()
processes BASE.  Here, we should only generate a forward declaration, if
necessary.

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

Indeed.

Until now, having a separate visit_type_NAME_fields() function was
pretty pointless: it was only called from visit_type_NAME().  Now, it's
also called from visit_type_UNION() when NAME is UNION's base.  Bonus:
we don't duplicate the code visiting NAME's members into every union
using it as base (this is what the commit message refers to when it says
"it lets us share code").

Can we do the same for structs, please?

The nice thing about generating code is that you don't have to
copy/paste so much to get all the duplication your heart desires.

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

Scratch the idea, I was confused.

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

Keeping the implicit tag implicit by not including it in local_members
was a conscious design decision, but I'm quite open to revisit it.

But perhaps we don't have to!  We got simple unions because "first past
the post": they solved immediate needs.  When other needs arose, flat
unions got bolted on.  We kept simple unions because the bolted-on flat
unions are bothersome to define.  If we can fuse struct and union types
into a reasonably pleasant variant record type, we may not need a simple
union sugar anymore.