[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 05/18] qapi: Test for various name collisions
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v7 05/18] qapi: Test for various name collisions |
Date: |
Thu, 01 Oct 2015 13:51:30 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> 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.
>
> This patch focuses on C struct members, and does not consider
> collisions between commands and events (affecting C function
> names), or even collisions between generated C type names with
> user type names (for things like automatic FOOList struct
> representing array types or FOOKind for an implicit enum).
>
> There are two types of struct collisions we want to catch:
> 1) Collision between two keys in a JSON object. qapi.py prevents
> that within a single struct (see duplicate-key), but it is
"(see test duplicate-key)" or maybe "(see test duplicate-key.json)".
> possible to have collisions between a type's members and its
> base type's members (struct-base-clash, struct-base-clash-deep,
> flat-union-cycle), and its flat union variant members
(existing tests struct-base-clash, struct-base-clash-deep, new test
flat-union-cycle)
> (flat-union-clash-member).
(new test flat-union-clash-member)
> 2) Collision between two members of the C struct that is generated
> for a given QAPI type:
> a) Multiple QAPI names map to the same C name (args-name-clash)
(new test args-name-clash)
> b) A QAPI name maps to a C name that is used for another purpose
> (flat-union-clash-branch, struct-base-clash-base,
> union-clash-data). We already fixed some such cases in commit
(new tests ...)
> 0f61af3e and 1e6c1616, but more remain.
> c) Two C names generated for other purposes clash
> (alternate-clash, union-clash-branches, union-clash-type,
> flat-union-clash-type)
(updated test alternate-clash, new tests ...)
>
> Ultimately, if we need to have a flat union where an enum value
Suggest "where a tag value", to make it clear it's not just any enum.
> clashes with a base member name, we could change the generator to
> name the union (using 'foo.u.value' rather than 'foo.value') or
> otherwise munge the C name corresponding to enum tag values. But
Suggest to drop enum here.
> unless such a need arises, it will probably be easier to just
> forbid these collisions.
>
> Some of these negative tests will be deleted later, and positive
> tests added to qapi-schema-test.json in their place, when the
> generator code is reworked to avoid particular code generation
> collisions in class 2).
>
> [Note that viewing this patch with git rename detection enabled
> may see some confusion due to renaming some tests while adding
> others, but where the content is similar enough that git picks
> the wrong pre- and post-patch files to associate]
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v7: rework commit message again, even more comments in tests, yet
> another rename (union-clash-members is now union-clash-branches)
> v6: rework commit message, alter test names and comments to be
> more descriptive, repurpose flat-union-cycle to better match its
> name (and not duplicate what the renamed flat-union-clash-branch
> tests), add new tests (union-clash-type, flat-union-clash-type)
> that detect an assertion failures
[...]
> diff --git a/tests/qapi-schema/alternate-clash.err
> b/tests/qapi-schema/alternate-clash.err
> index 51bea3e..a475ab6 100644
> --- a/tests/qapi-schema/alternate-clash.err
> +++ b/tests/qapi-schema/alternate-clash.err
> @@ -1 +1 @@
> -tests/qapi-schema/alternate-clash.json:2: Alternate 'Alt1' member 'ONE'
> clashes with 'one'
> +tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' member 'a_b'
> clashes with 'a-b'
> diff --git a/tests/qapi-schema/alternate-clash.json
> b/tests/qapi-schema/alternate-clash.json
> index 3947935..6d73bc5 100644
> --- a/tests/qapi-schema/alternate-clash.json
> +++ 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.
> diff --git a/tests/qapi-schema/flat-union-branch-clash.out
> b/tests/qapi-schema/args-name-clash.err
> similarity index 100%
> rename from tests/qapi-schema/flat-union-branch-clash.out
> rename to tests/qapi-schema/args-name-clash.err
> diff --git a/tests/qapi-schema/args-name-clash.exit
> b/tests/qapi-schema/args-name-clash.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/args-name-clash.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/args-name-clash.json
> b/tests/qapi-schema/args-name-clash.json
> new file mode 100644
> index 0000000..9e8f889
> --- /dev/null
> +++ b/tests/qapi-schema/args-name-clash.json
> @@ -0,0 +1,5 @@
> +# C member name collision
> +# FIXME - This parses, but fails to compile, because the C struct is given
> +# two 'a_b' members. Either reject this at parse time, or munge the C names
> +# to avoid the collision.
> +{ 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } }
> diff --git a/tests/qapi-schema/args-name-clash.out
> b/tests/qapi-schema/args-name-clash.out
> new file mode 100644
> index 0000000..9b2f6e4
> --- /dev/null
> +++ b/tests/qapi-schema/args-name-clash.out
> @@ -0,0 +1,6 @@
> +object :empty
> +object :obj-oops-arg
> + member a-b: str optional=False
> + member a_b: str optional=False
> +command oops :obj-oops-arg -> None
> + gen=True success_response=True
> diff --git a/tests/qapi-schema/duplicate-key.err
> b/tests/qapi-schema/duplicate-key.err
> index 768b276..6d02f83 100644
> --- a/tests/qapi-schema/duplicate-key.err
> +++ b/tests/qapi-schema/duplicate-key.err
> @@ -1 +1 @@
> -tests/qapi-schema/duplicate-key.json:2:10: Duplicate key "key"
> +tests/qapi-schema/duplicate-key.json:3:10: Duplicate key "key"
> diff --git a/tests/qapi-schema/duplicate-key.json
> b/tests/qapi-schema/duplicate-key.json
> index 1b55d88..14ac0e8 100644
> --- a/tests/qapi-schema/duplicate-key.json
> +++ b/tests/qapi-schema/duplicate-key.json
> @@ -1,2 +1,3 @@
> +# QAPI cannot include the same key more than once in any {}
> { 'key': 'value',
> 'key': 'value' }
> diff --git a/tests/qapi-schema/flat-union-base-union.err
> b/tests/qapi-schema/flat-union-base-union.err
> index ede9859..28725ed 100644
> --- a/tests/qapi-schema/flat-union-base-union.err
> +++ b/tests/qapi-schema/flat-union-base-union.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-base-union.json:11: Base 'UnionBase' is not a
> valid struct
> +tests/qapi-schema/flat-union-base-union.json:14: Base 'UnionBase' is not a
> valid struct
> diff --git a/tests/qapi-schema/flat-union-base-union.json
> b/tests/qapi-schema/flat-union-base-union.json
> index 6a8ea68..98b4eba 100644
> --- a/tests/qapi-schema/flat-union-base-union.json
> +++ b/tests/qapi-schema/flat-union-base-union.json
> @@ -1,4 +1,7 @@
> -# we require the base to be a struct
> +# For now, we require the base to be a struct without variants
> +# TODO: It would be possible to allow a union as a base, as long as all
> +# permutations of QMP names exposed by base do not clash with any QMP
> +# member names added by local variants.
> { 'enum': 'TestEnum',
> 'data': [ 'value1', 'value2' ] }
> { 'struct': 'TestTypeA',
> diff --git a/tests/qapi-schema/flat-union-branch-clash.err
> b/tests/qapi-schema/flat-union-branch-clash.err
> deleted file mode 100644
> index f112766..0000000
> --- a/tests/qapi-schema/flat-union-branch-clash.err
> +++ /dev/null
> @@ -1 +0,0 @@
> -tests/qapi-schema/flat-union-branch-clash.json:10: Member name 'name' of
> branch 'value1' clashes with base 'Base'
> diff --git a/tests/qapi-schema/flat-union-clash-branch.err
> b/tests/qapi-schema/flat-union-clash-branch.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/flat-union-clash-branch.exit
> b/tests/qapi-schema/flat-union-clash-branch.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-branch.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/flat-union-clash-branch.json
> b/tests/qapi-schema/flat-union-clash-branch.json
> new file mode 100644
> index 0000000..e593336
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-branch.json
> @@ -0,0 +1,18 @@
> +# Flat union branch name collision
> +# FIXME: this parses, but then fails to compile due to a duplicate 'c_d'
> +# (one from the base member, the other from the branch name). We should
> +# either reject the collision at parse time, or munge the generated branch
> +# name to allow this to compile.
> +{ 'enum': 'TestEnum',
> + 'data': [ 'base', 'c-d' ] }
> +{ 'struct': 'Base',
> + 'data': { 'enum1': 'TestEnum', '*c_d': 'str' } }
> +{ 'struct': 'Branch1',
> + 'data': { 'string': 'str' } }
> +{ 'struct': 'Branch2',
> + 'data': { 'value': 'int' } }
> +{ 'union': 'TestUnion',
> + 'base': 'Base',
> + 'discriminator': 'enum1',
> + 'data': { 'base': 'Branch1',
> + 'c-d': 'Branch2' } }
> diff --git a/tests/qapi-schema/flat-union-clash-branch.out
> b/tests/qapi-schema/flat-union-clash-branch.out
> new file mode 100644
> index 0000000..8e0da73
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-branch.out
> @@ -0,0 +1,14 @@
> +object :empty
> +object Base
> + member enum1: TestEnum optional=False
> + member c_d: str optional=True
> +object Branch1
> + member string: str optional=False
> +object Branch2
> + member value: int optional=False
> +enum TestEnum ['base', 'c-d']
> +object TestUnion
> + base Base
> + tag enum1
> + case base: Branch1
> + case c-d: Branch2
> diff --git a/tests/qapi-schema/flat-union-clash-member.err
> b/tests/qapi-schema/flat-union-clash-member.err
> new file mode 100644
> index 0000000..152d402
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-member.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/flat-union-clash-member.json:10: Member name 'name' of
> branch 'value1' clashes with base 'Base'
> diff --git a/tests/qapi-schema/flat-union-branch-clash.exit
> b/tests/qapi-schema/flat-union-clash-member.exit
> similarity index 100%
> rename from tests/qapi-schema/flat-union-branch-clash.exit
> rename to tests/qapi-schema/flat-union-clash-member.exit
> diff --git a/tests/qapi-schema/flat-union-branch-clash.json
> b/tests/qapi-schema/flat-union-clash-member.json
> similarity index 85%
> rename from tests/qapi-schema/flat-union-branch-clash.json
> rename to tests/qapi-schema/flat-union-clash-member.json
> index 8fb054f..3d7e6c6 100644
> --- a/tests/qapi-schema/flat-union-branch-clash.json
> +++ b/tests/qapi-schema/flat-union-clash-member.json
> @@ -1,4 +1,4 @@
> -# we check for no duplicate keys between branches and base
> +# we check for no duplicate keys between branch members and base
Spelling out the clashing members wouldn't hurt.
> { 'enum': 'TestEnum',
> 'data': [ 'value1', 'value2' ] }
> { 'struct': 'Base',
> diff --git a/tests/qapi-schema/flat-union-clash-member.out
> b/tests/qapi-schema/flat-union-clash-member.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/flat-union-clash-type.err
> b/tests/qapi-schema/flat-union-clash-type.err
> new file mode 100644
> index 0000000..6e64d1d
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-type.err
> @@ -0,0 +1,16 @@
> +Traceback (most recent call last):
> + File "tests/qapi-schema/test-qapi.py", line 55, in <module>
> + schema = QAPISchema(sys.argv[1])
> + File "scripts/qapi.py", line 1116, in __init__
> + self.check()
> + File "scripts/qapi.py", line 1299, in check
> + ent.check(self)
> + File "scripts/qapi.py", line 962, in check
> + self.variants.check(schema, members, seen)
> + File "scripts/qapi.py", line 1024, in check
> + v.check(schema, self.tag_member.type, vseen)
> + File "scripts/qapi.py", line 1032, in check
> + QAPISchemaObjectTypeMember.check(self, schema, [], seen)
> + File "scripts/qapi.py", line 994, in check
> + assert self.name not in seen
> +AssertionError
> diff --git a/tests/qapi-schema/flat-union-clash-type.exit
> b/tests/qapi-schema/flat-union-clash-type.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-type.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/flat-union-clash-type.json
> b/tests/qapi-schema/flat-union-clash-type.json
> new file mode 100644
> index 0000000..eac51a4
> --- /dev/null
> +++ 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.
> +# the branch name 'type' within the union. We should either reject this,
> +# or munge the generated C to let it compile.
> +{ 'enum': 'TestEnum',
> + 'data': [ 'type' ] }
> +{ 'struct': 'Base',
> + 'data': { 'type': 'TestEnum' } }
> +{ 'struct': 'Branch1',
> + 'data': { 'string': 'str' } }
> +{ 'union': 'TestUnion',
> + 'base': 'Base',
> + 'discriminator': 'type',
> + 'data': { 'type': 'Branch1' } }
> diff --git a/tests/qapi-schema/flat-union-clash-type.out
> b/tests/qapi-schema/flat-union-clash-type.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/flat-union-cycle.err
> b/tests/qapi-schema/flat-union-cycle.err
> new file mode 100644
> index 0000000..9b7978a
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-cycle.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/flat-union-cycle.json:7: Base 'Union' is not a valid struct
> diff --git a/tests/qapi-schema/flat-union-cycle.exit
> b/tests/qapi-schema/flat-union-cycle.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-cycle.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/flat-union-cycle.json
> b/tests/qapi-schema/flat-union-cycle.json
> new file mode 100644
> index 0000000..c66c4c9
> --- /dev/null
> +++ 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?
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.
> +{ 'enum': 'Enum', 'data': [ 'okay' ] }
> +{ 'struct': 'Okay', 'data': { 'int': 'int' } }
> +{ 'union': 'Union', 'base': 'Union', 'discriminator': 'switch',
> + 'data': { 'okay': 'Okay' } }
> diff --git a/tests/qapi-schema/flat-union-cycle.out
> b/tests/qapi-schema/flat-union-cycle.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/qapi-schema-test.json
> b/tests/qapi-schema/qapi-schema-test.json
> index 6897a6e..c511338 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -32,11 +32,14 @@
> 'dict1': 'UserDefTwoDict' } }
>
> # for testing unions
> +# Among other things, test that a name collision between branches does
> +# not cause any problems (since only one branch can be in use at a time),
> +# by intentionally using two branches that both have a C member 'a_b'
> { 'struct': 'UserDefA',
> - 'data': { 'boolean': 'bool' } }
> + 'data': { 'boolean': 'bool', '*a_b': 'int' } }
>
> { 'struct': 'UserDefB',
> - 'data': { 'intb': 'int' } }
> + 'data': { 'intb': 'int', '*a-b': 'bool' } }
>
> { 'union': 'UserDefFlatUnion',
> 'base': 'UserDefUnionBase', # intentional forward reference
> diff --git a/tests/qapi-schema/qapi-schema-test.out
> b/tests/qapi-schema/qapi-schema-test.out
> index 1f6e858..28a0b3c 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -71,6 +71,7 @@ enum QEnumTwo ['value1', 'value2']
> prefix QENUM_TWO
> object UserDefA
> member boolean: bool optional=False
> + member a_b: int optional=True
> alternate UserDefAlternate
> case uda: UserDefA
> case s: str
> @@ -78,6 +79,7 @@ alternate UserDefAlternate
> enum UserDefAlternateKind ['uda', 's', 'i']
> object UserDefB
> member intb: int optional=False
> + member a-b: bool optional=True
> object UserDefC
> member string1: str optional=False
> member string2: str optional=False
> diff --git a/tests/qapi-schema/struct-base-clash-base.err
> b/tests/qapi-schema/struct-base-clash-base.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/struct-base-clash-base.exit
> b/tests/qapi-schema/struct-base-clash-base.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash-base.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/struct-base-clash-base.json
> b/tests/qapi-schema/struct-base-clash-base.json
> new file mode 100644
> index 0000000..0c84025
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash-base.json
> @@ -0,0 +1,9 @@
> +# Struct member 'base'
> +# FIXME: this parses, but then fails to compile due to a duplicate 'base'
> +# (one explicit in QMP, the other used to box the base class members).
> +# We should either reject the collision at parse time, or change the
> +# generated struct to allow this to compile.
> +{ 'struct': 'Base', 'data': {} }
> +{ 'struct': 'Sub',
> + 'base': 'Base',
> + 'data': { 'base': 'str' } }
> diff --git a/tests/qapi-schema/struct-base-clash-base.out
> b/tests/qapi-schema/struct-base-clash-base.out
> new file mode 100644
> index 0000000..e69a416
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash-base.out
> @@ -0,0 +1,5 @@
> +object :empty
> +object Base
> +object Sub
> + base Base
> + member base: str optional=False
> diff --git a/tests/qapi-schema/struct-base-clash-deep.err
> b/tests/qapi-schema/struct-base-clash-deep.err
> index e3e9f8d..f7a25a3 100644
> --- a/tests/qapi-schema/struct-base-clash-deep.err
> +++ b/tests/qapi-schema/struct-base-clash-deep.err
> @@ -1 +1 @@
> -tests/qapi-schema/struct-base-clash-deep.json:7: Member name 'name' clashes
> with base 'Base'
> +tests/qapi-schema/struct-base-clash-deep.json:10: Member name 'name' clashes
> with base 'Base'
> diff --git a/tests/qapi-schema/struct-base-clash-deep.json
> b/tests/qapi-schema/struct-base-clash-deep.json
> index 552fe94..fa873ab 100644
> --- a/tests/qapi-schema/struct-base-clash-deep.json
> +++ b/tests/qapi-schema/struct-base-clash-deep.json
> @@ -1,4 +1,7 @@
> -# we check for no duplicate keys with indirect base
> +# Reject attempts to duplicate QMP members
> +# Here, 'name' would have to appear twice on the wire, locally and
> +# indirectly for the grandparent base; the collision doesn't care that
> +# one instance is optional.
> { 'struct': 'Base',
> 'data': { 'name': 'str' } }
> { 'struct': 'Mid',
> diff --git a/tests/qapi-schema/struct-base-clash.err
> b/tests/qapi-schema/struct-base-clash.err
> index 3ac37fb..3a9f66b 100644
> --- a/tests/qapi-schema/struct-base-clash.err
> +++ b/tests/qapi-schema/struct-base-clash.err
> @@ -1 +1 @@
> -tests/qapi-schema/struct-base-clash.json:4: Member name 'name' clashes with
> base 'Base'
> +tests/qapi-schema/struct-base-clash.json:5: Member name 'name' clashes with
> base 'Base'
> diff --git a/tests/qapi-schema/struct-base-clash.json
> b/tests/qapi-schema/struct-base-clash.json
> index f2afc9b..11aec80 100644
> --- a/tests/qapi-schema/struct-base-clash.json
> +++ b/tests/qapi-schema/struct-base-clash.json
> @@ -1,4 +1,5 @@
> -# we check for no duplicate keys with base
> +# Reject attempts to duplicate QMP members
> +# Here, 'name' would have to appear twice on the wire, locally and for base.
> { 'struct': 'Base',
> 'data': { 'name': 'str' } }
> { 'struct': 'Sub',
> 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' } }
> diff --git a/tests/qapi-schema/union-clash-branches.out
> b/tests/qapi-schema/union-clash-branches.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/union-clash-data.err
> b/tests/qapi-schema/union-clash-data.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/union-clash-data.exit
> b/tests/qapi-schema/union-clash-data.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-data.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/union-clash-data.json
> b/tests/qapi-schema/union-clash-data.json
> new file mode 100644
> index 0000000..7308e69
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-data.json
> @@ -0,0 +1,7 @@
> +# Union branch 'data'
> +# FIXME: this parses, but then fails to compile due to a duplicate 'data'
> +# (one from the branch name, another as a filler to avoid an empty union).
> +# we should either detect the collision at parse time, or change the
> +# generated struct to allow this to compile.
> +{ 'union': 'TestUnion',
> + 'data': { 'data': 'int' } }
> diff --git a/tests/qapi-schema/union-clash-data.out
> b/tests/qapi-schema/union-clash-data.out
> new file mode 100644
> index 0000000..6277239
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-data.out
> @@ -0,0 +1,6 @@
> +object :empty
> +object :obj-int-wrapper
> + member data: int optional=False
> +object TestUnion
> + case data: :obj-int-wrapper
> +enum TestUnionKind ['data']
> diff --git a/tests/qapi-schema/union-clash-type.err
> b/tests/qapi-schema/union-clash-type.err
> new file mode 100644
> index 0000000..6e64d1d
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-type.err
> @@ -0,0 +1,16 @@
> +Traceback (most recent call last):
> + File "tests/qapi-schema/test-qapi.py", line 55, in <module>
> + schema = QAPISchema(sys.argv[1])
> + File "scripts/qapi.py", line 1116, in __init__
> + self.check()
> + File "scripts/qapi.py", line 1299, in check
> + ent.check(self)
> + File "scripts/qapi.py", line 962, in check
> + self.variants.check(schema, members, seen)
> + File "scripts/qapi.py", line 1024, in check
> + v.check(schema, self.tag_member.type, vseen)
> + File "scripts/qapi.py", line 1032, in check
> + QAPISchemaObjectTypeMember.check(self, schema, [], seen)
> + File "scripts/qapi.py", line 994, in check
> + assert self.name not in seen
> +AssertionError
> diff --git a/tests/qapi-schema/union-clash-type.exit
> b/tests/qapi-schema/union-clash-type.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-type.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/union-clash-type.json
> b/tests/qapi-schema/union-clash-type.json
> new file mode 100644
> index 0000000..38330b5
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-type.json
> @@ -0,0 +1,9 @@
> +# 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 union tag 'kind' and
Suggest "the simple union's implicit tag member 'kind' and"
> +# the branch name 'kind' within the union. We should either reject this,
> +# or munge the generated C to let it compile.
> +# TODO: Even when the generated C is switched to use 'type' rather than
> +# 'kind', to match the QMP spelling, the collision should still be detected.
> +{ 'union': 'TestUnion',
> + 'data': { 'kind': 'int', 'type': 'str' } }
> diff --git a/tests/qapi-schema/union-clash-type.out
> b/tests/qapi-schema/union-clash-type.out
> new file mode 100644
> index 0000000..e69de29
Found nothing that couldn't be touched up on commit.
- [Qemu-devel] [PATCH v7 15/18] qapi: Share gen_err_check(), (continued)
- [Qemu-devel] [PATCH v7 15/18] qapi: Share gen_err_check(), Eric Blake, 2015/10/08
- [Qemu-devel] [PATCH v7 02/18] qapi: Improve 'include' error message, Eric Blake, 2015/10/08
- [Qemu-devel] [PATCH v7 12/18] qapi: Consistent generated code: prefer common labels, Eric Blake, 2015/10/08
- [Qemu-devel] [PATCH v7 04/18] qapi: Clean up qapi.py per pep8, Eric Blake, 2015/10/08
- [Qemu-devel] [PATCH v7 07/18] qapi: Add tests for empty unions, Eric Blake, 2015/10/08
- [Qemu-devel] [PATCH v7 05/18] qapi: Test for various name collisions, Eric Blake, 2015/10/08
- Re: [Qemu-devel] [PATCH v7 05/18] qapi: Test for various name collisions, Eric Blake, 2015/10/08
- Re: [Qemu-devel] [PATCH v7 05/18] qapi: Test for various name collisions,
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH v7 05/18] qapi: Test for various name collisions, Eric Blake, 2015/10/08
- Re: [Qemu-devel] [PATCH v7 05/18] qapi: Test for various name collisions, Markus Armbruster, 2015/10/08
- Re: [Qemu-devel] [PATCH v7 05/18] qapi: Test for various name collisions, Eric Blake, 2015/10/08
- Re: [Qemu-devel] [PATCH v7 05/18] qapi: Test for various name collisions, Markus Armbruster, 2015/10/08
- Re: [Qemu-devel] [PATCH v7 05/18] qapi: Test for various name collisions, Eric Blake, 2015/10/08
- Re: [Qemu-devel] [PATCH v7 05/18] qapi: Test for various name collisions, Markus Armbruster, 2015/10/08
[Qemu-devel] [PATCH v7 13/18] qapi: Consistent generated code: prefer common indentation, Eric Blake, 2015/10/08
[Qemu-devel] [PATCH v7 11/18] qapi: Consistent generated code: prefer visitor 'v', Eric Blake, 2015/10/08
[Qemu-devel] [RFC PATCH v7 18/18] qapi: Use gen_err_check() in more places, Eric Blake, 2015/10/08
[Qemu-devel] [RFC PATCH v7 17/18] qapi: Simplify gen_visit_fields() error handling, Eric Blake, 2015/10/08