Re: [Qemu-devel] [PATCH v9 05/17] qapi: Unbox base members

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 10/21/2015 07:34 AM, Markus Armbruster wrote:
>
>>>>> @@ -218,9 +216,11 @@ static void channel_event(int event,
>>>>> SpiceChannelEventInfo *info)
>>>>>      }
>>>>>
>>>>>      if (info->flags & SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT) {
>>>>> -        add_addr_info(client->base, (struct sockaddr *)&info->paddr_ext,
>>>>> +        add_addr_info((SpiceBasicInfo *)client,
>>>>
>>>> Ah, you're already exploiting the ability to cast to the base type!
>>>
>>> Absolutely :)
>>>
>>>>
>>>> Idea: should we generate a type-safe macro or inline function for this?
>>>
>>> Hmm. DO_UPCAST() (and its more powerful underlying container_of())
>>> doesn't fit here, because we inlined the fields rather than directly
>>> including the base.
>> 
>> Yes, because it results in slightly more readable code: always simply
>> p->m instead of things like p->base.base.m when m is inherited (which is
>> usually of no concern when it's used).
>> 
>
>>> There's also the ugly approach of exposing things in a dual naming
>>> system via an anonymous union and struct:
>>>
>>> struct Child {
>>>     union {
>>>         struct {
>>>             int i;
>>>         };
>>>         Parent base;
>>>     };
>>>     bool b;
>>> };
>>>
>>> which would allow 'child->i' to be the same storage as 'child->base.i',
>>> so that clients can use the short spelling when accessing fields but
>>> also have handy access to the base member for DO_UPCAST().  I'm not sure
>>> I want to go there, though.
>> 
>> Seems to clever for its own sake :)
>
> I agree (and even though I'm using a similar hack in 7/17 for the same
> purpose, I get rid of it as soon as possible in 16/17).
>
>>> But while such a representation would add compiler type-safety (hiding
>>> the cast in generated code, where we can prove the generator knew it was
>>> safe, and so that clients don't have to cast), it also adds verbosity.
>>> I can't think of any representation that would be shorter than making
>>> the clients do the cast, short of using a container rather than inline
>>> approach.  Even foo(qapi_baseof_Child(child), blah) is longer than
>>> foo((Parent *)child, blah).
>>>
>>> Preferences?
>> 
>> The least verbose naming convention for a conversion function I can
>> think of right now is TBase(), where T is the name of a type with a
>> base.  Compare:
>> 
>>     foo((Parent *)child, blah)
>>     foo(ChildBase(child), blah)
>> 
>> Tolerable?  Worthwhile?
>
> The verbosity is then determined by how long the child name is (where
> the cast depended on the parent name)

Child vs. parent is probably a wash on average.  Cases like
BlockdevOptions inheriting BlockdevOptionsBase become slightly more
concise (but need a rename to avoid the clash), cases like VncServerInfo
inheriting from VncBasicInfo take a few more characters.

> What happens with multiple inheritance?
>
> If we have Grandparent -> Parent -> Child, then it should be possible to
> cast to both bases:
>
> (Grandparent *)child
> (Parent *)child
>
> with your scheme, getting a child to grandparent would have to look like
> either of:
>
> ParentBase(ChildBase(child))
> ChildBaseBase(child)

I'd start with the former.  If it becomes annoyingly verbose, we can
still define additional conversion functions.  I doubt it'll be
necessary.

> Or, think what happens if we originally have Grandparent -> Child in one
> version of qemu, then inject Parent in the middle in another - the QMP
> can still be back-compat, and the direct casts and member variable
> references in C still work, but any code using ChildBase() no longer
> works (returning Parent* instead of Grandparent*).

The compiler will lead us to the places that need updating.

Could be regarded a feature, even: it encourages us to review whether
these places should use the new intemediate type.

> So the only thing I can think of is to output some name that includes
> both the child type and parent type name (to make it obvious which
> conversion is being done):
>
> qapi_Child_Grandparent(child)
> qapi_Child_Parent(child)

We don't feel the need to encode a function's return type in its name
elsewhere, so why do it here?

> At this point, I'm leaning towards client casts, just because of the
> verbosity, but I'll at least try the generated type-safe functions in
> v10 to see how bad it really is.  A patch to discuss will make it easier
> to decide whether to paint this bikeshed.

I try to avoid casts, especially pointer casts, because it's essentially
telling the compiler "shut up, I know what I'm doing".  But I'm
unwilling to trade much readability for fewer Let's see how things work
out here.