Re: [Qemu-devel] [PATCH RFC v2 07/47] qapi: Generate a nicer struct for flat unions

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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.

I do like it.  Perhaps something like

    struct UserDefFlatUnion
    {
        /* Members inherited from UserDefUnionBase: */
        char *string;
        EnumOne enum1;
        /* Own members: */
        ...
    };

>>         /* 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).

It surprised me, too.

>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  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 <address@hidden>

Thanks!