Re: [Qemu-devel] [PATCH V8 02/10] qapi script: add check for duplicated key

[Eric Blake]

On 02/27/2014 04:09 AM, Wenchao Xia wrote:
> From: Wenchao Xia <address@hidden>
> 
> It is bad that same key was specified twice, especially when a union have

s/have/has/

> two branches with same condition. This patch can prevent it.
> 
> Signed-off-by: Wenchao Xia <address@hidden>
> Signed-off-by: Wenchao Xia <address@hidden>

Again, the double S-o-B is odd.

> ---
>  scripts/qapi.py                      |    2 ++
>  tests/Makefile                       |    3 ++-
>  tests/qapi-schema/duplicate-key.err  |    1 +
>  tests/qapi-schema/duplicate-key.exit |    1 +
>  tests/qapi-schema/duplicate-key.json |    2 ++
>  5 files changed, 8 insertions(+), 1 deletions(-)
>  create mode 100644 tests/qapi-schema/duplicate-key.err
>  create mode 100644 tests/qapi-schema/duplicate-key.exit
>  create mode 100644 tests/qapi-schema/duplicate-key.json
>  create mode 100644 tests/qapi-schema/duplicate-key.out
> 

> +++ b/tests/qapi-schema/duplicate-key.json
> @@ -0,0 +1,2 @@
> +{ 'key': 'value',
> +  'key': 'value' }

This tests a duplicate key in a dictionary.  Since unions use
'data':{dictionary}, that means you caught duplicate branches within a
union.  Based on your test, your patch also has the nice effect of
catching duplicates unrelated to unions!  This test is of a top-level
struct; a bit more realistic might be:

{ 'command': 'foo',
  'command': 'foo', 'data': {} }

Meanwhile, should we also test for duplicates in non-dict locations,
such as:

{ 'enum': 'Foo', 'data': [ 'value', 'value' ] }

or even tests of duplicate top-level items, such as:

{ 'type': 'Dup', 'data': { 'field':'str' } }
{ 'type': 'Dup', 'data': { 'field':'str' } }

But I'm okay with that as a followup, in the interest of getting this
series in.

Reviewed-by: Eric Blake <address@hidden>

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

signature.asc
Description: OpenPGP digital signature