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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3783 bytes --]

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)

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)

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

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)

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.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]