Eric Blake <ebl...@redhat.com> writes:
> On 10/26/2015 11:07 AM, Markus Armbruster wrote:
>> Eric Blake <ebl...@redhat.com> writes:
>>
>>> We have two issues with our qapi union layout:
>>> 1) Even though the QMP wire format spells the tag 'type', the
>>> C code spells it 'kind', requiring some hacks in the generator.
>>> 2) The C struct uses an anonymous union, which places all tag
>>> values in the same namespace as all non-variant members. This
>>> leads to spurious collisions if a tag value matches a QMP name.
>>>
>>> Make the conversion to the new layout for qapi-visit.py.
>>
>> This part is nicely mechanical: ->kind becomes ->type, ->variant becomes
>> ->u.variant, and variants.tag_name or 'kind' becomes
>> variants.tag_member.name.
>>
>>> Also
>>> make a change in qapi.py to quit using the name 'kind', and
>>> the corresponding change in the testsuite.
>>
>> This isn't really part of "qapi-visit: Convert to new qapi union
>> layout". Could be made a separate patch, but I'd simply squash it into
>> the previous one "qapi: Start converting to new qapi union layout". See
>> also my remark inline.
>>
>
> It _was_ a part of the previous patch in v9 :)
Actually, it was in "[PATCH v9 08/17] tests: Convert to new qapi union
layout" (i.e. the next patch), not in "[PATCH v9 07/17] qapi: Start
converting to new qapi union layout" (the previous patch).
> [...]
>
>>> +++ b/scripts/qapi.py
>>> @@ -548,7 +548,7 @@ def check_union(expr, expr_info):
>>> base = expr.get('base')
>>> discriminator = expr.get('discriminator')
>>> members = expr['data']
>>> - values = {'MAX': '(automatic)', 'KIND': '(automatic)'}
>>> + values = {'MAX': '(automatic)', 'TYPE': '(automatic tag)'}
>>>
>>> # Two types of unions, determined by discriminator.
>>>
>>
>> Took me a minute to remember what's going on here. A simple union's tag
>> values, become an enum type. Here, we catch two differend kinds of
>> clashes:
>>
>> 1. Tag value with the implicitly defined enum member _MAX, i.e. a clash
>> in the enum member name space
>>
>> 2. Member of the (as of yet still) unnamed union holding the variants
>> with the tag variable type, formerly kind (i.e. a clash in the struct
>> member name space).
>>
>> The previous patch added type to the struct member name space without
>> updating clash detection.
>>
>> A future patch will remove the unnamed union, thus clash kind 2. It'll
>> also remove kind from the struct member name space, but that doesn't
>> matter.
>>
>> I believe the following would be slightly cleaner: amend the previous
>> patch to *add* key 'TYPE' to values. Drop both 'KIND' and 'TYPE' when
>> you remove the unnamed union.
>
> Okay, that works for me. Reserve additional namespace in 12, then drop
> the reservation in the later patch (now in subset C, see my comments to
> 24/25)...
>
>>
>>> diff --git a/tests/qapi-schema/union-clash-type.err
>>> b/tests/qapi-schema/union-clash-type.err
>>> index a5dead1..eca7c1d 100644
>>> --- a/tests/qapi-schema/union-clash-type.err
>>> +++ b/tests/qapi-schema/union-clash-type.err
>>> @@ -1 +1 @@
>>> -tests/qapi-schema/union-clash-type.json:8: Union 'TestUnion'
>>> member 'kind' clashes with '(automatic)'
>>> +tests/qapi-schema/union-clash-type.json:8: Union 'TestUnion'
>>> member 'type' clashes with '(automatic tag)'
>>
>> You might be able to avoid this change by adding 'TYPE' in the right
>> spot.
>
> ...and see if I can avoid churn in union-clash-type.json up until the
> point in subset C where it disappears. I'll see how it plays out.
> Expect v11 soon.
Thanks!
