[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 05/17] qapi: Unbox base members
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v9 05/17] qapi: Unbox base members |
Date: |
Wed, 21 Oct 2015 15:16:59 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v9 13/17] input: Convert to new qapi union layout, Eric Blake, 2015/10/16
[Qemu-devel] [PATCH v9 02/17] qapi: Reserve '*List' type names for arrays, Eric Blake, 2015/10/16
[Qemu-devel] [PATCH v9 07/17] qapi: Start converting to new qapi union layout, Eric Blake, 2015/10/16