Re: [Qemu-devel] [PATCH v5 06/28] qapi: Add some union tests

[Eric Blake]

On 03/27/2015 06:30 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> On 03/26/2015 07:18 AM, Markus Armbruster wrote:
>>> Eric Blake <address@hidden> writes:
>>>
>>>> Demonstrate that the qapi generator doesn't deal well with unions
>>>> that aren't up to par. Later patches will update the expected
>>>> reseults as the generator is made stricter.

Oh my. s/reseults/results/

Looks like I'll be doing a v6 to add R-b and clean up these nits, as it
is turning into too large a collection to ask a maintainer to do on my
behalf.  I'll try and split up some of the patches where I crammed
multiple things into one commit, as part of the respin.

>>>>
>>>> Of particular note, we currently allow 'base' without 'discriminator'
>>>> as a way to create a simple union with a base class.  However, none
>>>> of the existing QMP or QGA interfaces use it (it is exercised only
>>>> in the testsuite).
>>>
>>> qapi-code-gen.txt documents 'base' only for flat unions.  We should
>>> either document its use in simple unions, or outlaw it.
>>
>> I went with outlaw later in the series, and the rest of my commit
>> message was an attempt to explain my reasoning in that choice.  But I
>> can certainly try to improve the wording, if we need a respin.
> 
> If you respin, I suggest to add that it's undocumented.

Sure.


>> I'm hoping to add as a followup series a variant of simple unions that
>> is type-safe:
>>
>> [3]
>> { 'union':'SimpleAndSafe', 'discriminator':'MyEnum',
>>   'data':{ 'one':'str', 'two':'int' }}
>>
>> Hmm - that makes me wonder - do we support non-dict branches in a flat
>> union?  The usual use case of a flat union is that all dictionary keys
>> in the branch's dictionary are treated as keys at the top level of the
>> resulting overall union; but a non-dictionary branch has no keys to
>> flatten into the top level.
> 
> I think you just showed why non-dictionary branches make no sense.
> 
>>                              I may need to tweak a subsequent patch to
>> ensure that flat union branches can only use dictionaries (while simple
>> unions can use any type).
> 
> Yes, please.  Follow-up patch okay, if that's easier for you.

At this point, it may indeed be easier (keep the front half of the patch
down to the bare minimum, and just make the entire series longer, rather
than trying to rebase things into perfection).

>>> What do you mean by "anonymous inline base type"?
>>
>> [5] basically, that the following example could be legal shorthand...
>>
>>>
>>>> +{ 'enum': 'TestEnum',
>>>> +  'data': [ 'value1', 'value2' ] }
>>>> +{ 'type': 'TestTypeA',
>>>> +  'data': { 'string': 'str' } }
>>>> +{ 'type': 'TestTypeB',
>>>> +  'data': { 'integer': 'int' } }
>>>> +{ 'union': 'TestUnion',
>>>> +  'base': { 'enum1': 'TestEnum', 'kind': 'str' },
>>>> +  'discriminator': 'TestEnum',
> 
> Shouldn't this be 'discriminator': 'enum1'?

Yes.  Good catch.

> 
> Since there's nothing currently broken, I wouldn't label the question
> "should we allow an anonymous inline base type?" FIXME.

As part of my respin, I'll change any FIXME into something more benign
like TODO or RFC if it is merely intended to document a possible future
direction and not and existing bug to be corrected by the series (so
that the remaining additions of FIXME in the earlier part of the series
will all disappear by the end of the series).

>>> Since all my questions are about your intentions and rationale:
>>>
>>> Reviewed-by: Markus Armbruster <address@hidden>
> 
> R-by stands, but I encourage you to polish the commit message and drop
> the FIXME: from the comment in flat-union-bad-base.json.

I think we've racked up enough to warrant a respin; I'll keep your R-b
where the only things I touch are obviously trivial, but it may mean a
few patches come through without R-b.

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

signature.asc
Description: OpenPGP digital signature