[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 08/10] qapi: Allow anonymous base for flat un
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v4 08/10] qapi: Allow anonymous base for flat union |
Date: |
Tue, 08 Mar 2016 19:14:18 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 03/08/2016 09:23 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'.
>>> We already have several struct types that only exist to serve as
>>> a single flat union's base; the next commit will clean them up.
>>>
>
>>
>> I think you also
>>
>> * fixed a missing allow_optional=True in check_union()
>
> More on that below.
>
>>
>> * fixed a missing "non-optional" in qapi-code-gen.txt (mention in commit
>> message? separate patch?)
>>
>> * renamed readonly to read-only in an example in qapi-code-gen.txt to be
>> closer to the code (separate patch?)
>
> Could separate those two cleanups if it helps.
I'd like the fix recorded in git-log. For that, a mention in the commit
message would suffice, but I think I'd rather separate them.
>>> @@ -560,9 +562,10 @@ def check_union(expr, expr_info):
>>>
>>> # Else, it's a flat union.
>>> else:
>>> - # The object must have a string member 'base'.
>>> + # The object must have a string or dictionary 'base'.
>>> check_type(expr_info, "'base' for union '%s'" % name,
>>> - base, allow_metas=['struct'])
>>> + base, allow_dict=True, allow_optional=True,
>>> + allow_metas=['struct'])
>>
>> This is where you added allow_optional=True.
>
> allow_optional only matters if allow_dict is True. We have places where
> we allow a dict but do not allow optionals (namely, simple unions); but
> where we are creating an anonymous type, we want to allow optionals at
> the same time we allow a dict. I think this is just a case where the
> commit message needs to be beefed up.
Perhaps not even that. I'm spoiled by your meticulous patch versioning,
and when once in a blue moon I spot something you didn't announce there,
I take note.
>>> +A flat union definition avoids nesting on the wire, and specifies a
>>> +set of common members that occur in all variants of the union. The
>>> +'base' key must specifiy either a type name (the type must be a
>>> +struct, not a union), or a dictionary representing an anonymous type.
>>> +All branches of the union must be complex types, and the top-level
>>> +members of the union dictionary on the wire will be combination of
>>> +members from both the base type and the appropriate branch type (when
>>> +merging two dictionaries, there must be no keys in common). The
>>> +'discriminator' member must be the name of a non-optional enum-typed
>>
>> This is where you supplied the missing "non-optional".
>>
>>> +member of the base struct.
>>>
>>
>> And below, you rename readonly to read-only.
>
> They touch the same paragraph, but I can separate them if it would make
> this patch feel cleaner.
- Re: [Qemu-devel] [PATCH v4 01/10] qapi: Assert in places where variants are not handled, (continued)
[Qemu-devel] [PATCH v4 03/10] qapi: Make c_type() more OO-like, Eric Blake, 2016/03/05
[Qemu-devel] [PATCH v4 02/10] qapi: Fix command with named empty argument type, Eric Blake, 2016/03/05
[Qemu-devel] [PATCH v4 08/10] qapi: Allow anonymous base for flat union, Eric Blake, 2016/03/05
[Qemu-devel] [PATCH v4 10/10] qapi: Populate info['name'] for each entity, Eric Blake, 2016/03/05
[Qemu-devel] [PATCH v4 09/10] qapi: Use anonymous bases in QMP flat unions, Eric Blake, 2016/03/05
[Qemu-devel] [PATCH v4 07/10] qapi: Don't special-case simple union wrappers, Eric Blake, 2016/03/05