Re: [Qemu-devel] [PATCH v2 14/19] qapi: Don't special-case simple union wrappers

[Eric Blake]

On 03/03/2016 03:59 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Simple unions were carrying a special case that hid their 'data'
>> QMP member from the resulting C struct, via the hack method
>> QAPISchemaObjectTypeVariant.simple_union_type().  But using the
>> work we started by unboxing flat union and alternate branches, we
>> expose the simple union's implicit type in qapi-types.h as an
>> anonymous type, and drop our last use of the hack.
>>

>> All clients of simple unions have to adjust from using su->u.member
>> to now using su->u.member.data;
> 
> By now, a reader not familiar with the code may wonder why this is an
> improvement.  It is, because
> 
> * it removes an asymmetry between QAPI's QMP side and QAPI's C side
>   (both now have 'data'), and
> 
> * it hopefully turns simple unions into sugar for flat unions as
>   described in qapi-code-gen.txt, where before their equivalence only
>   applied to the QMP side, not to the C side.
> 
> And that's well worth having to type .data in a few places.
> 
> Can we work that into the commit message?

Yes, definitely.


>> +++ b/scripts/qapi-types.py
>> @@ -122,14 +122,24 @@ def gen_variants(variants):
>>                  c_name=c_name(variants.tag_member.name))
>>
>>      for var in variants.variants:
>> -        # Ugly special case for simple union TODO get rid of it
>> -        simple_union_type = var.simple_union_type()
>> -        typ = simple_union_type or var.type
>> -        ret += mcgen('''
>> +        if (isinstance(var.type, QAPISchemaObjectType) and
>> +                var.type.is_implicit()):
> 
> Uh, this condition is exactly var.type.simple_union_type() != None.  I'm
> afraid we still have a special case.

The isinstance() is necessary because of alternates - a builtin type
branch to an alternate is implicit, but must be emitted directly, only
object types can be unboxed.  And, down the road, if we DO add anonymous
branches to a flat union, then this condition will also work for that
anonymous branch (in fact, I have it in my local tree, just not part of
this series).  Yes, that's the part of the commit message you said I
could drop, but I'll have to come up with some way to highlight that
potential in the commit message.

> 
> Special treatment for simple unions: instead of a member
> 

Rather, special treatment for an implicit object branch (right now, only
simple unions have implicit object branches, but an anonymous branch to
a flat union would also qualify for this treatment):

>     TypeOfBranch name_of_branch;
> 
> we generate one
> 
>     struct {
>         TypeOfBranch data;
>     } name_of_branch;
> 

> Without the special case, we'd get
> 
>     typedef struct :obj-intList-wrapper :obj-intList-wrapper;
> 
>     struct :obj-intList-wrapper {
>         intList *data;
>     } :obj-intList-wrapper;
> 
>     struct UserDefNativeListUnion {
>         UserDefNativeListUnionKind type;
>         union { /* union tag is @type */
>             :obj-intList-wrapper integer;
>             [more of the same...]
>         } u;
>     }
> 
> except QAPISchemaObjectType.c_name() would refuse to cooperate in
> creating this nonsense.
> 
> Conclusion: you replace one special case by another one.  The
> improvement is in that the new special case is less special.  Instead of
> "if simple union variant, do something else", we now have
> 
>     Use the C type corresponding to the type, except when it's an
>     implicit object type, use an anonymous struct type, because we don't
>     have a C type then.

Yes, exactly.  Words I should use in my commit message :)

> 
> Should we have a C type even then?  We'd need to give it a reserved
> name.

I found it easier to inline an anonymous struct than to think about how
to create a reserved name.  Maybe that decision of mine can be revisited.

> 
> On first glance, the new special case is just as special at the old one:
> it applies to simple unions.  But that's not necessarily so.  We could
> make use of it elsewhere if we wanted.  We'd have to factor the code out
> of the "for variants" loop, of course.  In other words, it's still
> special, but its specialness is less arbitrary.  That's why it's an
> improvement.
> 
> Next is the visit update for this change of the type layout.
> 
> Note that direct_name is only used when !typ.is_implicit(), and
> implicit_name is only used when typ.is_implicit().
> 
> Further note that despite its name, gen_visit_members_call() doesn't
> generate a call when typ.is_implicit().
> 
> Separate function for implicit type?

By the end of the series, we have two callers of the helper; if we split
to two helpers, then both callers have to test for .is_implicit() (and
it gets worse if I find a third place to use this helper in a later
patch).  My goal was to make the helper do as much as possible to
simplify the callers, but I got stuck at how to pass the difference
between a direct-use prefix vs. an implicit-use prefix.

Again, maybe the idea of creating a named C type for implicit types
would make this simpler.

> After:
> 
>        if typ.is_empty():
>            pass
>        elif typ.is_implicit():
>            assert implicit_name
>            assert not typ.variants
>            ret += gen_visit_members(typ.members, prefix=implicit_name)
>        else:
>            ret += mcgen('''
>        visit_type_%(c_type)s_members(v, %(c_name)s, &err);
>    ''',
>                         c_type=typ.c_name(), c_name=direct_name)
> 
> Special treatment for member of implicit type: generate inline code to
> visit its members, because visit_type_T_members() doesn't exist then.
> 
> Should it exist?

Only if we create a named C type for uses of implicit types.

>> +++ b/backends/baum.c
>> @@ -567,7 +567,7 @@ static CharDriverState *chr_baum_init(const char *id,
>>                                        ChardevReturn *ret,
>>                                        Error **errp)
>>  {
>> -    ChardevCommon *common = backend->u.braille;
>> +    ChardevCommon *common = backend->u.braille.data;
>>      BaumDriverState *baum;
>>      CharDriverState *chr;
>>      brlapi_handle_t *handle;
> 
> Many trivial updates like this one.  The only interesting question is
> whether you got them all.  What did you do to find them?

The compiler caught most of them.  For a few others, particularly under
net/, it was search and replace (basically, the compiler warned me about
some uses of NetClientOptions now being different, so I then grepped for
ALL uses of NetClientOptions to pick up the ones that I'm not set up to
compile).

I may have missed something, but it is a compiler error, so someone
would flag it pretty quickly if they are set up to compile code that I
am not.

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

signature.asc
Description: OpenPGP digital signature