On 07/01/2015 02:21 PM, Markus Armbruster wrote:
> The struct generated for a flat union is weird: the members of its
> base are at the end, except for the union tag, which is renamed to
> 'kind' and put at the beginning.
> 

> Change to put all base members at the beginning, unadulterated.  Not
> only is this easier to understand, it also permits casting the flat
> union to its base, if that should become useful.
> 
> We now generate:
> 
>     struct UserDefFlatUnion
>     {

It might be worth tweaking the generator to output a C comment either
here (at the start of the larger struct)...

>         char *string;
>         EnumOne enum1;

...or here (at the end of the base struct) mentioning that
UserDefFlatUnion can be cast into its base struct UserDefUnionBase, to
make it easier when reading the generated code to trace back to that
"inheritance" relationship.  Right now, there is nothing in the
generated UserDefFlatUnion that points you back to the qapi relationship
of a base class.  But it's not a show-stopper if you don't like my
suggestion.

>         /* union tag is EnumOne enum1 */
>         union {
>             void *data;
>             UserDefA *value1;
>             UserDefB *value2;
>             UserDefB *value3;
>         };
>     };

Only impact in files generated for qemu was to struct BlockdevOptions,
in the files qapi-types.[ch], and I agree that it is an improvement.
Oddly enough, it seems none of the C code cared about the field being
renamed from 'kind' to 'driver' (I guess that's because a lot of our use
of the blockdev code is not directly through the generated C structs,
but through QObject dictionary queries).

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-types.py           | 32 ++++++++++++++++++--------------
>  scripts/qapi-visit.py           |  7 +++++--
>  tests/test-qmp-input-visitor.c  |  2 +-
>  tests/test-qmp-output-visitor.c |  2 +-
>  4 files changed, 25 insertions(+), 18 deletions(-)
> 

> +''',
> +                name=name)
> +    if base:
> +        base_fields = find_struct(base)['data']
> +        ret += generate_struct_fields(base_fields)

> -    if base:
> -        assert discriminator
> -        base_fields = find_struct(base)['data'].copy()
> -        del base_fields[discriminator]
> -        ret += generate_struct_fields(base_fields)

I also like the fact that you no longer modify the base_fields array
(because you are no longer floating a single renamed element
out-of-order compared to the rest of the base), and therefore avoid the
need for a .copy().

Reviewed-by: Eric Blake <eblake@redhat.com>

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