Eric Blake <ebl...@redhat.com> 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).