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/24/2015 06:01 AM, Markus Armbruster wrote:
>
>>> 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: */
>>         ...
>>     };
>
> What about nested types?  Whatever we pick has to look good when
> indirect bases are involved. Suppose we have Base -> Mid -> Derived as a
> struct inheritance.  If we generate a comment at both start and end of
> visiting a base class (and visit base classes recursively), we'd have
> something like:
>
> struct Derived
> {
>     /* Members inherited from Mid */
>     /* Members inherited from Base */
>     int one;
>     /* End members inherited from Base */
>     int two;
>     /* End members inherited from Mid */
>     /* own members */
>     int three;
>     ...
> };

Documenting the direct ancestor in the code is more important than the
others, because you can always peruse the direct ancestor to find the
next one.

> Don't know if that helps you think about it more, but now I'm just
> painting a bikeshed that just adds the tail comment.  Here's the
> resulting RFC:
>
> http://thread.gmane.org/gmane.comp.emulators.qemu/353204
>
> and the resulting generated structs:
>
>  struct BlockdevOptionsQcow2 {
> -    BlockdevOptionsGenericCOWFormat *base;
> +    BlockdevRef *file;
> +    /* End fields inherited from BlockdevOptionsGenericFormat. */
> +    bool has_backing;
> +    BlockdevRef *backing;
> +    /* End fields inherited from BlockdevOptionsGenericCOWFormat. */
>      bool has_lazy_refcounts;
>      bool lazy_refcounts;
> ...

This and other followup patches are encouraging.  I'll gladly review
them when I can spare the cycles.  Right now, I need to focus them on
introspection.