On 03/03/2016 06:04 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> writes: > >> Rather than requiring all flat unions to explicitly create >> a separate base struct, we can allow the qapi schema to specify >> the common members via an inline dictionary. This is similar to >> how commands can specify an inline anonymous type for its 'data', > > Suggest to end the sentence here, and then... > >> and matches the fact that our code base already has several >> flat unions that had to create a separate base type that is used >> nowhere but in the union. > > "We already have several struct types that only exist to serve as a > single flat union's base. The next commits will clean them up." > > Replace "them" by "some" if you don't clean them all up. > > It's a nice step towards having a variant record type in the schema > language similar to what we have in introspection. >
>> @@ -63,7 +62,8 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s
>> *obj, Error **errp)
>> c_name=c_name(name))
>>
>> if base:
>> - ret += gen_visit_members_call(base, '(%s *)obj' % base.c_name())
>> + ret += gen_visit_members_call(base, 'qapi_%s_base(obj)' %
>> c_name(name),
>
> I started at this for several minutes until I could guess what's going
> on here.
>
> The old code works fine when the type isn't implicit.
>
> When it is, it fails the assertion in base.c_name(), even though
> gen_visit_members_call() is not going to use its value.
>
> You hack around it by passing 'qapi_NAME_base(obj)' instead.
>
> If NAME isn't implicit, the function exists, and does the same as the
> expression it replaces.
>
> If NAME is implicit, the function doesn't exist, but
> gen_visit_members_call() doesn't care, because it doesn't use the
> argument then.
>
> Ugh! More evidence that we better not munge the two cases together into
> one function.
Even with my v4 work towards exposing implicit types as a concrete
struct, I'm still not creating qapi_NAME_base(obj) for objects with an
implicit type. But '(_obj_FOO_base *)FOO' works well for a base with a
concrete implicit base type.
>> @@ -354,7 +355,7 @@ code generator can ensure that branches exist for all
>> values of the
>> enum (although the order of the keys need not match the declaration of
>> the enum). In the resulting generated C data types, a flat union is
>> represented as a struct with the base member fields included directly,
>> -and then a union of structures for each branch of the struct.
>> +and then a union of pointers to structures for each branch of the struct.
>
> Uh, that became wrong in commit 544a373 already, didn't it?
>
> Is that a bug in PATCH 3 then?
Yes, and fixed up accordingly in my v3 respin. (I think it was some
rebase conflicts that I resolved incorrectly at some point).
>> +++ b/tests/qapi-schema/qapi-schema-test.json
>> @@ -75,14 +75,10 @@
>> 'base': 'UserDefZero',
>> 'data': { 'string': 'str', 'enum1': 'EnumOne' } }
>>
>> -{ 'struct': 'UserDefUnionBase2',
>> - 'base': 'UserDefZero',
>> - 'data': { 'string': 'str', 'enum1': 'QEnumTwo' } }
>> -
>> # this variant of UserDefFlatUnion defaults to a union that uses fields with
>> # allocated types to test corner cases in the cleanup/dealloc visitor
>> { 'union': 'UserDefFlatUnion2',
>> - 'base': 'UserDefUnionBase2',
>> + 'base': { 'string': 'str', 'enum1': 'QEnumTwo' },
>
> You lost member 'integer' from the base's base. Harmless (I think), but
> visible when you compare generated output.
Easy enough to keep.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
