Eric Blake <ebl...@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.