Eric Blake <ebl...@redhat.com> writes: > On 10/20/2015 06:09 AM, Markus Armbruster wrote: >> Eric Blake <ebl...@redhat.com> writes: >> >>> Rather than storing a base class as a pointer to a box, just >>> store the fields of that base class in the same order, so that >>> a child struct can be safely cast to its parent. > > ^^^^ > >>> Compare to the earlier commit 1e6c1616a "qapi: Generate a nicer >>> struct for flat unions". >> >> Should we mention the struct can be cast to its base? > > As in the ^^^ sentence above? Or did you mean somewhere additionally in > the comments in the code below?
Wish I could read %-} Sorry for the noise! >>> -def gen_struct_fields(members): >>> +def gen_struct_fields(members, base, nested=False): >>> ret = '' >>> + if base: >>> + if not nested: >>> + ret += mcgen(''' >>> + /* Members inherited from %(c_name)s: */ >>> +''', >>> + c_name=base.c_name()) >>> + ret += gen_struct_fields(base.local_members, base.base, True) >>> + if not nested: >>> + ret += mcgen(''' >>> + /* Own members: */ >>> +''') >> >> Before: gen_struct_fields() emits *own* fields. >> >> After: it emits *all* fields. >> >> Since the signature changes, all callers are visible in the patch, and >> can be reviewed. >> >> Code looks a bit awkward, but I don't have better ideas. Perhaps we can >> do better when we fuse gen_struct() and gen_union(). > > I haven't tried to do that fusion yet; but we are probably quite close > to it. I'll see whether it is worth adding on as an 18/17 in this > subset of the series, or saving for later. Doesn't feel urgent to me. >> This is gen_union(). >> >>> ''', >>> c_name=c_name(name)) >>> if base: >>> - ret += mcgen(''' >>> - /* Members inherited from %(c_name)s: */ >>> -''', >>> - c_name=c_name(base.name)) >>> - ret += gen_struct_fields(base.members) >>> - ret += mcgen(''' >>> - /* Own members: */ >>> -''') >>> + ret += gen_struct_fields([], base) >>> else: >>> ret += mcgen(''' >>> %(c_type)s kind; >> >> Before: emit base fields, then own fields. >> >> After: emit all fields. Good, but please mention in the commit message >> that you're touching gen_union(). You could also do this part in a >> separate patch, but that's probably overkill. > > Sure, I can improve the commit message, and maybe split the patch. The > idea at play is that both structs and unions want to emit all fields, as > well as strategic comments about which fields are inherited, so a shared > function is ideal for that; this patch moved code from gen_union() into > gen_struct_fields() so that gen_struct() could reuse it. I feel there's a variant record caught inside our struct and union types, struggling to get out. It's already out in introspection. Your patch is a welcome step towards getting it out elsewhere. >>> if base: >>> - ret += gen_visit_implicit_struct(base) >>> + ret += gen_visit_struct_fields(base.name, base.base, >>> + base.local_members) >>> >>> ret += mcgen(''' >>> >> >> This change looks innocent enough on first glance, but it's actually >> quite hairy. >> >> = Before = >> >> The visit_type_FOO_fields() are generated in QAPISchema visit order, >> i.e. right when QAPISchemaObjectType 'FOO' is visited. >> >> The visit_type_implicit_FOO() are generated on demand, right before >> their first use. It's used by visit_type_STRUCT_fields() when STRUCT >> has base FOO, and by visit_type_UNION() when flat UNION has a member >> FOO. >> >> Aside: only flat unions, because simple unions are an ugly special case >> here. To be unified. >> >> Generating visit_type_implicit_FOO() on demand makes sense, because the >> function isn't always needed. >> >> Since visit_type_implicit_FOO() calls visit_type_FOO_fields(), and the >> former can be generated before the latter, we may need a forward >> declaration. struct_fields_seen is used to detect this. See commit >> 8c3f8e7. >> >> = After = >> >> visit_type_implicit_FOO() is now used only when flat UNION has a member >> FOO. May need a forward declaration of visit_type_FOO_fields() anyway. >> >> visit_type_FOO_fields() is now also generated on demand, right before >> first use other than visit_type_implicit_FOO(). Weird. >> >> The resulting code motion makes the change to generated code difficult >> to review. >> >> Generating visit_type_FOO_fields() on demand makes less sense, because >> the function is always needed. All it can accomplish is saving a few >> forward declarations, at the cost of making gen_visit_struct_fields() >> recursive, which begs the recursion termination question, and less >> uniform generated code. >> >> The naive answer to the recursion termination question is that types >> can't contain themselves, therefore we can't ever get into a cycle. >> That begs the next question: why do we need the if name in >> struct_fields_seen conditional then? We do need it (turning it into an >> assertion promptly fails it), but I can't tell why offhand. >> >> I'm sure I could figure out why this works, but I don't want to. Let's >> keep the code simple instead: keep generating visit_type_FOO_fields() in >> QAPISchema visit order, emit necessary forward declarations. >> >> Suggest to factor the code to emit a forward declaration out of >> gen_visit_implicit_struct() and reuse it in gen_visit_struct_fields(). > > Sounds like I need to split this patch then, anyways. I'll see what I > can come up with for v10. Looking forward to it. I'll continue reviewing v9 as time permits. >>> +++ b/tests/qapi-schema/qapi-schema-test.json >>> @@ -40,6 +40,10 @@ >>> 'data': { 'string0': 'str', >>> 'dict1': 'UserDefTwoDict' } } >>> >>> +# ensure that we don't have an artificial collision on 'base' >>> +{ 'struct': 'UserDefThree', >>> + 'base': 'UserDefOne', 'data': { 'base': 'str' } } >>> + > >> This is the positive test I challenged above. > > I can drop it if you don't like it; I felt better with it, but as you > say, it only proves that 'base' is usable as a member name, and not that > someone won't pick some other name when refactoring back into a boxed > base. See also my comments below about using containers (rather than > boxes or inlined members), where we may need to deal with the naming > clash anyways. > >>> @@ -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). Drawbacks of inlining the base include: * You have to keep the copies consistent. Too much bother in hand-written code, but trivially safe in generated code like ours. * Loss of the obvious safe way to convert to base: &p->base * Loss of the conventional way to convert from base: container_of(). I remember seeing conversion to base in your patch, but not the other way round. > Below, I'm using this qapi: > {'struct':'Parent', 'data':{'i':'int'}} > {'struct':'Child', 'base':'Parent', 'data':{'b':'bool'}} > > What we have without this patch is a box that requires separate > allocation (two layers of pointers, the boxing that we don't want): > > struct Child { > Parent *base; > bool b; > }; > > And this patch takes things to be completely inlined (since that's what > we did earlier with flat unions), so that there is no intermediate > structure in the child (and thus nothing for DO_UPCAST() to grab): > > struct Child { > /* Inherited fields from Parent */ > int i; > /* own fields */ > bool b; > }; > > But there is a third possible layout (if we do it, we should do it for > flat unions as well), which is using the base as a container rather than > a box, where at least we don't need separate allocation: > > struct Child { > Parent base; > bool b; > }; > > or similarly, but in longhand: > > struct Child { > struct { > int i; > } base; > bool b; > }; > > but then we are back to having to avoid a collision with the C name > 'base' with a QMP name in own fields, and all client code has to spell > out child->base.i (instead of the boxed child->base->i, or the inlined > child->i). Yes. > 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 :) > Taking your idea from another review message, you mentioned that we > could allow 'base' by tweaking c_name() to munge the user's member > 'base' into 'q_base', while reserving 'base' for ourselves; or we could > use '_base' for our own use, which shouldn't collide with user's names > (user names should not start with underscore except for downstream names > which have double-underscore). Or use '_b' instead of 'base' - the > point remains that if we want to avoid a collision but still use the > container approach, we could pick the C name appropriately. But > ultimately, we should either commit to the name munging pattern, or > outlaw the name that we plan to use internally, or stick to inlined > members that don't require munging in the first place. > > Meanwhile, if we decide that we like the convenience of inlined data, > you are correct in the idea that we could have the qapi generator > automatically spit out an appropriate type-specific upcast inline > function for each type with a base. So given the same example, this > might mean: > > static inline Parent *qapi_Child_to_Parent(Child *child) { > return (Parent*)child; > } > > 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? Note: *Base instead of *_base not because I like StudlyCaps (I do not), but for consistency with *List and *Kind. >> Turned out nicely, I anticipated more churn. > > Yes, that was my feeling as well - we haven't quite used base classes in > enough places to make the conversion painful - but the longer we wait, > the more usage of base classes will sneak into .json files making a > future conversion to a new layout more painful.