Re: [Qemu-devel] [PATCH v2 1/2] qapi: allow empty branches in flat unions

[Eric Blake]

On 05/29/2018 11:41 AM, Anton Nefedov wrote:
> It often happens that just a few discriminator values imply extra data in
> a flat union. Existing checks did not make possible to leave other values
> uncovered. Such cases had to be worked around by either stating a dummy
> (empty) type or introducing another (subset) discriminator enumeration.
>
> Both options create redundant entities in qapi files for little profit.
>
> With this patch it is not necessary anymore to add designated union
> fields for every possible value of a discriminator enumeration.
>
> Signed-off-by: Anton Nefedov <address@hidden>
> ---
>   docs/devel/qapi-code-gen.txt                        |  7 ++++---
>   scripts/qapi/common.py                              | 11 ++++-------
>   scripts/qapi/types.py                               |  4 +++-
>   scripts/qapi/visit.py                               | 17 +++++++++++++----
>   tests/qapi-schema/flat-union-incomplete-branch.err  |  1 -
>   tests/qapi-schema/flat-union-incomplete-branch.exit |  2 +-
>   tests/qapi-schema/flat-union-incomplete-branch.out  | 14 ++++++++++++++

Generally, when converting a negative test to a positive test, I prefer 
to instead drop the negative test and put the positive test as part of 
tests/qapi-schema/qapi-schema-test.json - because that file has the 
additional coverage of proving that the compiler is still happy with the 
generated code (we don't compile any of the other positive tests).

>   7 files changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index b9b6eab..a3ea450 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -496,9 +496,10 @@ Resulting in these JSON objects:
>   
>   Notice that in a flat union, the discriminator name is controlled by
>   the user, but because it must map to a base member with enum type, the
> -code generator can ensure that branches exist for all values of the
> -enum (although the order of the keys need not match the declaration of
> -the enum).  In the resulting generated C data types, a flat union is
> +code generator ensures that branches match the existing values of the
> +enum. The order of the keys need not match the declaration of the enum.
> +The keys need not cover all possible enum values.

Maybe mention that omitted enum values are still valid branches that 
merely add no additional members to the data type.

> +In the resulting generated C data types, a flat union is
>   represented as a struct with the base members included directly, and
>   then a union of structures for each branch of the struct.
>   

Otherwise seems reasonable to me.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org