On 03/03/2016 03:59 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> 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
