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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 09/29/2015 04:21 PM, Eric Blake wrote:
>> Expose some weaknesses in the generator: we don't always forbid
>> the generation of structs that contain multiple members that map
>> to the same C or QMP name.  This has already been marked FIXME in
>> qapi.py in commit d90675f, but having more tests will make sure
>> future patches produce desired behavior; and updating existing
>> patches to better document things doesn't hurt, either.  Some of
>> these collisions are already caught in the old-style parser
>> checks, but ultimately we want all collisions to be caught in the
>> new-style QAPISchema*.check() methods.
>> 
>
>> diff --git a/tests/qapi-schema/union-clash-branches.err
>> b/tests/qapi-schema/union-clash-branches.err
>> new file mode 100644
>> index 0000000..005c48d
>> --- /dev/null
>> +++ b/tests/qapi-schema/union-clash-branches.err
>> @@ -0,0 +1 @@
>> +tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion'
>> member 'a_b' clashes with 'a-b'
>> diff --git a/tests/qapi-schema/union-clash-branches.exit
>> b/tests/qapi-schema/union-clash-branches.exit
>> new file mode 100644
>> index 0000000..d00491f
>> --- /dev/null
>> +++ b/tests/qapi-schema/union-clash-branches.exit
>> @@ -0,0 +1 @@
>> +1
>> diff --git a/tests/qapi-schema/union-clash-branches.json
>> b/tests/qapi-schema/union-clash-branches.json
>> new file mode 100644
>> index 0000000..31d135f
>> --- /dev/null
>> +++ b/tests/qapi-schema/union-clash-branches.json
>> @@ -0,0 +1,5 @@
>> +# Union branch name collision
>> +# Reject a union that would result in a collision in generated C names (this
>> +# would try to generate two enum values 'TEST_UNION_KIND_A_B').
>> +{ 'union': 'TestUnion',
>> +  'data': { 'a-b': 'int', 'a_b': 'str' } }
>
> Hmm. This test is very similar to the existing union-bad-branch (I guess
> it's poor name is why I didn't notice it before). But that test only
> covered 'one' vs. 'ONE' (no clash in the C struct, just in the generated
> MyUnionKind enum type); while this test also clashes in the C struct.
> Don't know if it is worth a v8 to clean up the duplication, or if we
> just save it for a followup patch (namely, where I try to move errors
> into the QAPISchema.check() methods); I found the issue while playing
> with v5 15/46.

Your choice.  The rather minor issues I found in this series could all
be touched up on commit (assuming consensus on what to do).