| Message ID | 1445898903-12082-10-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > A previous patch (commit 1e6c1616) made it possible to > directly cast from a qapi flat union type to its base type. > However, it requires the use of a C cast, which turns off > compiler type-safety checks. Add inline type-safe wrappers > named qapi_FOO_base() for any union type FOO that has a base, > which can be used for a safer upcast, and enhance the > testsuite to cover the new functionality. A future patch > will extend the upcast support to structs. > > Note that C makes const-correct upcasts annoying because > it lacks overloads; these functions cast away const so that > they can accept user pointers whether const or not, and the > result in turn can be assigned to normal or const pointers. > Alternatively, this could have been done with macros, but > those tend to not have quite as much type safety. Well, the macros can be made just as type-safe, but the result is either somewhat ugly and using gcc-isms, or very ugly and unhygienic. I'd write something like "type-safe macros are hairy, and not worthwhile here." > This patch just adds upcasts. None of our code needed to > downcast from a base qapi class to a child. Actually, none of our code needs to upcast unions, either. Only the new tests do. Code that updasts structs exist, but it doesn't use this patch's upcasts until later. Suggest to amend the first paragraph: A previous patch (commit 1e6c1616) made it possible to directly cast from a qapi flat union type to its base type. However, it requires the use of a C cast, which turns off compiler type-safety checks. Fortunately, no such casts exist just, yet. Regardless, add inline type-safe wrappers named qapi_FOO_base() for any union type FOO that has a base, which can be used for a safer upcast, and enhance the testsuite to cover the new functionality. A future patch will extend the upcast support to structs, where such casts do exist already. > Also, in the > case of grandchildren (such as BlockdevOptionsQcow2), the > caller will need to call two functions to get to the inner > base (although it wouldn't be too hard to generate a > qapi_FOO_base_base() if desired). If a user changes qapi > to alter the base class hierarchy, such as going from > 'A -> C' to 'A -> B -> C', it will change the type of > 'qapi_C_base()', and the compiler will point out the places > that are affected by the new base. > > One alternative was proposed, but was deemed too ugly to use > in practice: the generators could output redundant > information using anonymous types: > | struct Child { > | union { > | struct { > | Type1 parent_member1; > | Type2 parent_member2; > | }; > | Parent base; > | }; > | }; > With that ugly proposal, for a given qapi type, obj->member > and obj->base.member would refer to the same storage; allowing > convenience in working with members without needing 'base.' > allowing typesafe upcast without needing a C cast by accessing > '&obj->base', and allowing downcasts from the parent back to > the child possible through container_of(obj, Child, base). > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v11: only support unions for this patch, add testsuite coverage > v10: new patch Patch looks good. I can touch up the commit message in my tree.
On 10/27/2015 01:46 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> A previous patch (commit 1e6c1616) made it possible to >> directly cast from a qapi flat union type to its base type. >> However, it requires the use of a C cast, which turns off >> compiler type-safety checks. Add inline type-safe wrappers >> named qapi_FOO_base() for any union type FOO that has a base, >> which can be used for a safer upcast, and enhance the >> testsuite to cover the new functionality. A future patch >> will extend the upcast support to structs. >> >> Note that C makes const-correct upcasts annoying because >> it lacks overloads; these functions cast away const so that >> they can accept user pointers whether const or not, and the >> result in turn can be assigned to normal or const pointers. >> Alternatively, this could have been done with macros, but >> those tend to not have quite as much type safety. > > Well, the macros can be made just as type-safe, but the result is either > somewhat ugly and using gcc-isms, or very ugly and unhygienic. > > I'd write something like "type-safe macros are hairy, and not worthwhile > here." Sure, that works for me. > >> This patch just adds upcasts. None of our code needed to >> downcast from a base qapi class to a child. > > Actually, none of our code needs to upcast unions, either. Only the new > tests do. Code that updasts structs exist, but it doesn't use this > patch's upcasts until later. > > Suggest to amend the first paragraph: > > A previous patch (commit 1e6c1616) made it possible to directly cast > from a qapi flat union type to its base type. However, it requires > the use of a C cast, which turns off compiler type-safety checks. > Fortunately, no such casts exist just, yet. s/ just,/, just/ > > Regardless, add inline type-safe wrappers named qapi_FOO_base() for > any union type FOO that has a base, which can be used for a safer > upcast, and enhance the testsuite to cover the new functionality. > > A future patch will extend the upcast support to structs, where such > casts do exist already. > Maybe s/casts/conversions/ - because as of this patch, it is still a conversion via foo->base rather than (Base *)foo (it's the next patch that gets rid of base, and therefore needs either the cast or the wrapper). > > Patch looks good. I can touch up the commit message in my tree. Sure, your proposed wording + touchups is fine.
Eric Blake <eblake@redhat.com> writes: > On 10/27/2015 01:46 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> A previous patch (commit 1e6c1616) made it possible to >>> directly cast from a qapi flat union type to its base type. >>> However, it requires the use of a C cast, which turns off >>> compiler type-safety checks. Add inline type-safe wrappers >>> named qapi_FOO_base() for any union type FOO that has a base, >>> which can be used for a safer upcast, and enhance the >>> testsuite to cover the new functionality. A future patch >>> will extend the upcast support to structs. >>> >>> Note that C makes const-correct upcasts annoying because >>> it lacks overloads; these functions cast away const so that >>> they can accept user pointers whether const or not, and the >>> result in turn can be assigned to normal or const pointers. >>> Alternatively, this could have been done with macros, but >>> those tend to not have quite as much type safety. >> >> Well, the macros can be made just as type-safe, but the result is either >> somewhat ugly and using gcc-isms, or very ugly and unhygienic. >> >> I'd write something like "type-safe macros are hairy, and not worthwhile >> here." > > Sure, that works for me. Sold. >>> This patch just adds upcasts. None of our code needed to >>> downcast from a base qapi class to a child. >> >> Actually, none of our code needs to upcast unions, either. Only the new >> tests do. Code that updasts structs exist, but it doesn't use this >> patch's upcasts until later. >> >> Suggest to amend the first paragraph: >> >> A previous patch (commit 1e6c1616) made it possible to directly cast >> from a qapi flat union type to its base type. However, it requires >> the use of a C cast, which turns off compiler type-safety checks. >> Fortunately, no such casts exist just, yet. > > s/ just,/, just/ Yes, thanks. >> >> Regardless, add inline type-safe wrappers named qapi_FOO_base() for >> any union type FOO that has a base, which can be used for a safer >> upcast, and enhance the testsuite to cover the new functionality. >> >> A future patch will extend the upcast support to structs, where such >> casts do exist already. >> > > Maybe s/casts/conversions/ - because as of this patch, it is still a > conversion via foo->base rather than (Base *)foo (it's the next patch > that gets rid of base, and therefore needs either the cast or the wrapper). Sold. >> Patch looks good. I can touch up the commit message in my tree. > > Sure, your proposed wording + touchups is fine.
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 40e9f79..f9fcf15 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -98,6 +98,19 @@ struct %(c_name)s { return ret +def gen_upcast(name, base): + # C makes const-correctness ugly. We have to cast away const to let + # this function work for both const and non-const obj. + return mcgen(''' + +static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj) +{ + return (%(base)s *)obj; +} +''', + c_name=c_name(name), base=base.c_name()) + + def gen_alternate_qtypes_decl(name): return mcgen(''' @@ -267,6 +280,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): if variants: assert not members # not implemented self.decl += gen_union(name, base, variants) + # TODO Use gen_upcast on structs too, once they have sane layout + if base: + self.decl += gen_upcast(name, base) else: self.decl += gen_struct(name, base, members) self._gen_type_cleanup(name) diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index 8941963..da21709 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -347,6 +347,7 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data, Visitor *v; Error *err = NULL; UserDefFlatUnion *tmp; + UserDefUnionBase *base; v = visitor_input_test_init(data, "{ 'enum1': 'value1', " @@ -360,6 +361,10 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data, g_assert_cmpstr(tmp->string, ==, "str"); g_assert_cmpint(tmp->integer, ==, 41); g_assert_cmpint(tmp->value1->boolean, ==, true); + + base = qapi_UserDefFlatUnion_base(tmp); + g_assert(&base->enum1 == &tmp->enum1); + qapi_free_UserDefFlatUnion(tmp); }
A previous patch (commit 1e6c1616) made it possible to directly cast from a qapi flat union type to its base type. However, it requires the use of a C cast, which turns off compiler type-safety checks. Add inline type-safe wrappers named qapi_FOO_base() for any union type FOO that has a base, which can be used for a safer upcast, and enhance the testsuite to cover the new functionality. A future patch will extend the upcast support to structs. Note that C makes const-correct upcasts annoying because it lacks overloads; these functions cast away const so that they can accept user pointers whether const or not, and the result in turn can be assigned to normal or const pointers. Alternatively, this could have been done with macros, but those tend to not have quite as much type safety. This patch just adds upcasts. None of our code needed to downcast from a base qapi class to a child. Also, in the case of grandchildren (such as BlockdevOptionsQcow2), the caller will need to call two functions to get to the inner base (although it wouldn't be too hard to generate a qapi_FOO_base_base() if desired). If a user changes qapi to alter the base class hierarchy, such as going from 'A -> C' to 'A -> B -> C', it will change the type of 'qapi_C_base()', and the compiler will point out the places that are affected by the new base. One alternative was proposed, but was deemed too ugly to use in practice: the generators could output redundant information using anonymous types: | struct Child { | union { | struct { | Type1 parent_member1; | Type2 parent_member2; | }; | Parent base; | }; | }; With that ugly proposal, for a given qapi type, obj->member and obj->base.member would refer to the same storage; allowing convenience in working with members without needing 'base.' allowing typesafe upcast without needing a C cast by accessing '&obj->base', and allowing downcasts from the parent back to the child possible through container_of(obj, Child, base). Signed-off-by: Eric Blake <eblake@redhat.com> --- v11: only support unions for this patch, add testsuite coverage v10: new patch --- scripts/qapi-types.py | 16 ++++++++++++++++ tests/test-qmp-input-visitor.c | 5 +++++ 2 files changed, 21 insertions(+)