Re: [Qemu-devel] [PATCH v7 05/18] qapi: Test for various name collisions

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3018 bytes --]

On 10/01/2015 05:51 AM, Markus Armbruster wrote:

>> +++ b/tests/qapi-schema/alternate-clash.json
>> @@ -1,3 +1,8 @@
>> -# we detect C enum collisions in an alternate
>> +# Alternate branch name collision
>> +# Reject an alternate that would result in a collision in generated C
>> +# names (this would try to generate two enum values 'ALT1_KIND_A_B').
>> +# TODO: In the future, if alternates are simplified to not generate
>> +# the implicit Alt1Kind enum, we would still have a collision with the
>> +# resulting C union trying to have two members named 'a_b'.
>>  { 'alternate': 'Alt1',
>> -  'data': { 'one': 'str', 'ONE': 'int' } }
>> +  'data': { 'a-b': 'str', 'a_b': 'int' } }
> 
> Ah, you're making the test slightly more robust.  Works for me.

I just noticed we lack coverage for:

{ 'alternate': 'Alt', 'data': { 'type': 'int', 'other': 'str' } }

(tries to create a C struct with two members named 'type', even after my
v5 patches change to a simpler 'qtype_code type'). Can be done in my
later patches that simplify alternates if we don't want a v8 spin of
this part of the series.


>> +++ b/tests/qapi-schema/flat-union-clash-type.json
>> @@ -0,0 +1,15 @@
>> +# Flat union branch 'type'
>> +# FIXME: this triggers an assertion failure. But even with that fixed, we
>> +# would have a clash in generated C, between the outer tag 'type' and
> 
> "outer tag"?  I guess you mean the member type we inherit from Base.

Yes.


>> +++ b/tests/qapi-schema/flat-union-cycle.json
>> @@ -0,0 +1,8 @@
>> +# Ensure that we have a sane error message for attempts at self-inheritance
>> +# This test currently fails because we don't permit a union base, but
>> +# even if we relax things to allow that in the future (see
>> +# flat-union-base-union), self-inheritance should still fail.
> 
> Do we have a test for the simpler case of a struct inheriting from
> itself?

Not here, but in v5 16/46. That's because it asserts, but while it was
easy to fix up in the QAPISchema.check(), I did not find it worth the
churn to fix it up in the ad hoc parse code just to rip it back out
later, and the QAPISchema.check() code requires several scaffolding
patches (so it wasn't as easy as fixing the union 'type' clash asserts).
 Tracking an assertion failure for any more than one patch at a time is
horrible (as any other change to qapi.py changes line numbers that
affect the assertion failure).

> 
> I believe us merging struct and union types into a single object type is
> more likely than us implementing union bases.  If I'm correct, we won't
> need this test.

Maybe, but even then, we still have to decide if a base object can have
variants.

> 
> Found nothing that couldn't be touched up on commit.

Your suggestions for wording tweaks are fine; I'm okay if you want to
tweak it for your pull request instead of asking me for a v8.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]