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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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.

Considering we're at v7, I'd like to get this series committed sooner
rather than later, and that means avoiding adding more functionality to
it.  Only advice; Wenchao is of course free to add whatever he wants :)

My first preference is to drop the code enforcing all enum values are
covered from the patch for now.  No new code to review.  Doesn't
preclude adding the code back later.

I can also live with keeping the enforcing code.  We'd have to watch out
for dummy fields until we implement support for {}.