Re: [Qemu-devel] [PATCH v5 10/28] qapi: Segregate anonymous unions into alternates in generator

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 03/26/2015 08:47 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> Special-casing 'discriminator == {}' for handling anonymous unions
>>> is getting awkward; since this particular type is not always a
>>> dictionary on the wire, it is easier to treat it as a completely
>>> different class of type, "alternate", so that if a type is listed
>>> in the union_types array, we know it is not an anonymous union.
>>>
>>> This patch just further segregates union handling, to make sure that
>>> anonymous unions are not stored in union_types, and splitting up
>>> check_union() into separate functions.  A future patch will change
>>> the qapi grammar, and having the segregation already in place will
>>> make it easier to deal with the distinct meta-type.
>>>
>>> Signed-off-by: Eric Blake <address@hidden>
>>> ---
>
>>> @@ -535,7 +546,8 @@ def find_struct(name):
>>>
>>>  def add_union(definition):
>>>      global union_types
>>> -    union_types.append(definition)
>>> +    if definition.get('discriminator') != {}:
>>> +        union_types.append(definition)
>>>
>>>  def find_union(name):
>>>      global union_types
>> 
>> This is the only unobvious hunk.
>> 
>> union_types is used only through find_union.  The hunk makes
>> find_union(N) return None when N names an anonymous union.
>> 
>> find_union() is used in two places:
>> 
>> * find_alternate_member_qtype()
>> 
>>   Patched further up.  It really wants only non-anonymous unions, and
>>   this change to find_union() renders its check for anonymous unions
>>   superfluous.  Good.
>> 
>> * generate_visit_alternate()
>> 
>>   Asserts that each member's type is either a built-in type, a complex
>>   type, a union type, or an enum type.
>> 
>>   The change relaxes the assertion not to trigger on anonymous union
>>   types.  Why is that okay?
>
> No, the change tightens the assertion so that it will now fail on an
> anonymous union nested as a branch of another anonymous union (where
> before it could silently pass the assertion), because the anonymous
> union is no longer found by find_union().  And this is okay because the
> earlier change to find_alternate_member_qtype means that we don't allow
> nested anonymous unions, so making the assertion fail if an anonymous
> union gets through anyway is correct.

Thanks for unconfusing me.

Reviewed-by: Markus Armbruster <address@hidden>