Eric Blake <ebl...@redhat.com> 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 <arm...@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 <ebl...@redhat.com>
Thanks!
