[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 16/19] qapi: Allow anonymous base for flat un

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v2 16/19] qapi: Allow anonymous base for flat union
Date: Fri, 4 Mar 2016 07:32:54 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0

On 03/03/2016 06:04 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]