| Message ID | 1446052473-19170-10-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > Add positive tests to qapi-schema-test for things that were > made possible by recent patches but which caused compile errors > due to collisions prior to that point. > > This includes: > Use of a member 'base' in a struct with a base class > Use of a member name ending in 'Kind' or 'List' > Use of a type name starting with 'has_' > Use of a type named 'u' > Use of a union branch name of 'u' > Use of a union branch name starting with 'has_' > Use of a union branch name of 'type' > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v8: new, but collects portions of subset B v10 patches 2, 3, and > 16 and subset C v7 patch 6 that were deferred to later. > > It might be worth dropping or simplifying this patch, depending > on how many corner cases we actually want to test. Let's go through your list: * Use of a member 'base' in a struct with a base class 'base' is no longer special, and testing it is as useful as testing any other non-special member name. I'd drop it. Or am I missing something? * Use of a member name ending in 'Kind' or 'List' These aren't special as member names, but they are reserved type names. The test ensures we don't accidentally reserve them as member names. Low probability * low damage = very low risk. But since you wrote the test already, we can just as well keep it. * Use of a type name starting with 'has_' * Use of a type named 'u' * Use of a union branch name of 'u' * Use of a union branch name starting with 'has_' Similarly: these are only reserved as member names, very low risk, but why not keep the test. * Use of a union branch name of 'type' As far as I know, 'type' isn't reserved anywhere, it's just the name of a simple union's implicit tag member. Testing a tag value doesn't clash with 'type' is as useful as testing any other non-variant member name. I'd drop it. Or am I missing something? Anything else being tested here?
On 11/03/2015 09:43 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Add positive tests to qapi-schema-test for things that were >> made possible by recent patches but which caused compile errors >> due to collisions prior to that point. >> >> This includes: >> Use of a member 'base' in a struct with a base class >> Use of a member name ending in 'Kind' or 'List' >> Use of a type name starting with 'has_' >> Use of a type named 'u' >> Use of a union branch name of 'u' >> Use of a union branch name starting with 'has_' >> Use of a union branch name of 'type' >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> --- >> v8: new, but collects portions of subset B v10 patches 2, 3, and >> 16 and subset C v7 patch 6 that were deferred to later. >> >> It might be worth dropping or simplifying this patch, depending >> on how many corner cases we actually want to test. > > Let's go through your list: > > * Use of a member 'base' in a struct with a base class > > 'base' is no longer special, and testing it is as useful as testing > any other non-special member name. I'd drop it. Or am I missing > something? Broken prior to commit ddf2190, and no reservation exists. I don't mind dropping it, especially since it quite distinct from the other grouping of collision testing. > > * Use of a member name ending in 'Kind' or 'List' > > These aren't special as member names, but they are reserved type > names. The test ensures we don't accidentally reserve them as member > names. Low probability * low damage = very low risk. But since you > wrote the test already, we can just as well keep it. > > * Use of a type name starting with 'has_' > * Use of a type named 'u' Never have been broken, but this adds insurance that we don't break it (and that our reservations added earlier aren't too greedy). > * Use of a union branch name of 'u' Broken prior to commit e4ba22b3; also proves that the reservation in 5359baf is not too greedy. > * Use of a union branch name starting with 'has_' Broken prior to commit e4ba22b3; also proves that the reservation in 9fb081e is not too greedy. > > Similarly: these are only reserved as member names, very low risk, but > why not keep the test. > > * Use of a union branch name of 'type' Broken prior to commit e4ba22b3. Doesn't have a reservation, so... > > As far as I know, 'type' isn't reserved anywhere, it's just the name > of a simple union's implicit tag member. Testing a tag value doesn't > clash with 'type' is as useful as testing any other non-variant member > name. I'd drop it. Or am I missing something? ...I could indeed drop this part of the test, on the same grounds as dropping the 'base' non-collision tests. > > Anything else being tested here? > I think that covers it. There's also one more positive test added in 10/17, when I rework the layout of alternate types (at that point, it is possible to have an alternate branch named 'max' because it no longer collides with a generated enum _MAX value).
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 44638da..4354604 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -47,6 +47,10 @@ 'data': { 'string0': 'str', 'dict1': 'UserDefTwoDict' } } +# ensure that we don't have an artificial collision on 'base' +{ 'struct': 'UserDefThree', + 'base': 'UserDefOne', 'data': { 'base': 'str' } } + # dummy struct to force generation of array types not otherwise mentioned { 'struct': 'ForceArrays', 'data': { 'unused1':['UserDefOne'], 'unused2':['UserDefTwo'], @@ -113,6 +117,22 @@ 'sizes': ['size'], 'any': ['any'] } } +# Even though 'u' and 'has_*' are forbidden as struct member names, they +# should still be valid as a type or union branch name. And although +# '*Kind' and '*List' are forbidden as type names, they should not be +# forbidden as a member or branch name. Flat union branches do not +# collide with base members. +{ 'enum': 'EnumName', 'data': [ 'value1', 'has_a', 'u', 'type' ] } +{ 'struct': 'has_a', 'data': { 'MyKind': 'int', 'MyList': ['int'], + 'value1': 'EnumName' } } +{ 'union': 'u', 'data': { 'u': 'uint8', 'myKind': 'has_a', + 'myList': 'has_a', 'has_a': 'has_a' } } +{ 'union': 'UnionName', 'base': 'has_a', 'discriminator': 'value1', + 'data': { 'value1': 'UserDefZero', 'has_a': 'UserDefZero', + 'u': 'UserDefZero', 'type': 'UserDefZero' } } +{ 'alternate': 'AltName', 'data': { 'type': 'int', 'u': 'bool', + 'myKind': 'has_a' } } + # testing commands { 'command': 'user_def_cmd', 'data': {} } { 'command': 'user_def_cmd1', 'data': {'ud1a': 'UserDefOne'} } diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 786024e..29c46da 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -22,6 +22,8 @@ object :obj-guest-get-time-arg member b: int optional=True object :obj-guest-sync-arg member arg: any optional=False +object :obj-has_a-wrapper + member data: has_a optional=False object :obj-int16List-wrapper member data: int16List optional=False object :obj-int32List-wrapper @@ -46,6 +48,8 @@ object :obj-uint32List-wrapper member data: uint32List optional=False object :obj-uint64List-wrapper member data: uint64List optional=False +object :obj-uint8-wrapper + member data: uint8 optional=False object :obj-uint8List-wrapper member data: uint8List optional=False object :obj-user_def_cmd1-arg @@ -57,6 +61,11 @@ alternate AltIntNum case i: int case n: number enum AltIntNumKind ['i', 'n'] +alternate AltName + case type: int + case u: bool + case myKind: has_a +enum AltNameKind ['type', 'u', 'myKind'] alternate AltNumInt case n: number case i: int @@ -84,6 +93,7 @@ event EVENT_D :obj-EVENT_D-arg object Empty1 object Empty2 base Empty1 +enum EnumName ['value1', 'has_a', 'u', 'type'] enum EnumOne ['value1', 'value2', 'value3'] object EventStructOne member struct1: UserDefOne optional=False @@ -105,6 +115,13 @@ object TestStruct member integer: int optional=False member boolean: bool optional=False member string: str optional=False +object UnionName + base has_a + tag value1 + case value1: UserDefZero + case has_a: UserDefZero + case u: UserDefZero + case type: UserDefZero object UserDefA member boolean: bool optional=False member a_b: int optional=True @@ -158,6 +175,9 @@ object UserDefOptions member u16: uint16List optional=True member i64x: int optional=True member u64x: uint64 optional=True +object UserDefThree + base UserDefOne + member base: str optional=False object UserDefTwo member string0: str optional=False member dict1: UserDefTwoDict optional=False @@ -201,6 +221,17 @@ command guest-get-time :obj-guest-get-time-arg -> int gen=True success_response=True command guest-sync :obj-guest-sync-arg -> any gen=True success_response=True +object has_a + member MyKind: int optional=False + member MyList: intList optional=False + member value1: EnumName optional=False +object u + member type: uKind optional=False + case u: :obj-uint8-wrapper + case myKind: :obj-has_a-wrapper + case myList: :obj-has_a-wrapper + case has_a: :obj-has_a-wrapper +enum uKind ['u', 'myKind', 'myList', 'has_a'] command user_def_cmd None -> None gen=True success_response=True command user_def_cmd1 :obj-user_def_cmd1-arg -> None
Add positive tests to qapi-schema-test for things that were made possible by recent patches but which caused compile errors due to collisions prior to that point. This includes: Use of a member 'base' in a struct with a base class Use of a member name ending in 'Kind' or 'List' Use of a type name starting with 'has_' Use of a type named 'u' Use of a union branch name of 'u' Use of a union branch name starting with 'has_' Use of a union branch name of 'type' Signed-off-by: Eric Blake <eblake@redhat.com> --- v8: new, but collects portions of subset B v10 patches 2, 3, and 16 and subset C v7 patch 6 that were deferred to later. It might be worth dropping or simplifying this patch, depending on how many corner cases we actually want to test. --- tests/qapi-schema/qapi-schema-test.json | 20 ++++++++++++++++++++ tests/qapi-schema/qapi-schema-test.out | 31 +++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+)