Re: [Qemu-devel] [PATCH V7 04/11] qapi script: check correctness of discriminator values in union

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1589 bytes --]

On 02/21/2014 01:21 AM, Markus Armbruster wrote:

>> I'd still rather make it explicit that we KNOW that this branch of the
>> union has no additional options:
>>
>> { 'union': 'FooOptions',
>>   'base': 'CommonFooOptions',
>>   'discriminator': 'type',
>>   'data': { 'plain': {},
>>             'bells': 'BellsOptions',
>>             'whistles': 'WhistlesOptions' } }
>>
>> to show that we explicitly thought about all the cases.  We don't
>> currently have any such unions with an empty branch, but it would be
>> worth documenting in the qapi text and explicitly testing that it works
>> if we intend to support this.
> 
> Fair point.  However, it requires 'plain': {} to work, and it doesn't in
> my testing.

> We should extend the generator to permit {} before we insist on unions
> covering all discriminator values explicitly.  Because if we don't,
> people will be compelled to add dummy fields.

So far, we have no one that should be omitting a branch.  But yes, I
agree that we should update the generator to allow an empty branch.

In the context of _this_ patch, requiring that all branches are covered
doesn't hurt until a future patch actually has to add a dummy field or
fix the generator.  I don't know if it's something Wenchao would like to
add in the next spin, or if we should just live this this patch being a
little over-ambitious on what it enforces in the absence of more generic
support for an explicit empty branch.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]