GNOME Bugzilla – Bug 508157
Add G_IMPLEMENT_INTERFACE_DYNAMIC
Last modified: 2009-11-26 18:31:33 UTC
When implementing an interface in a dynamic module using G_DEFINE_DYNAMIC_TYPE_EXTENDED you can't use G_IMPLEMENT_INTERFACE(). Would like something like: #define G_IMPLEMENT_INTERFACE_DYNAMIC(TYPE_IFACE, iface_init) { \ const GInterfaceInfo g_implement_interface_info = { \ (GInterfaceInitFunc) iface_init, NULL, NULL \ }; \ g_type_module_add_interface (type_module, g_define_type_id, TYPE_IFACE, &g_implement_interface_info); \ } Which is used like this: G_DEFINE_DYNAMIC_TYPE_EXTENDED (GHalVolume, g_hal_volume, G_TYPE_OBJECT, 0, G_IMPLEMENT_INTERFACE_DYNAMIC (G_TYPE_VOLUME, g_hal_volume_volume_iface_init))
(In reply to comment #0) > #define G_IMPLEMENT_INTERFACE_DYNAMIC(TYPE_IFACE, iface_init) { \ [...] > (GInterfaceInitFunc) iface_init, NULL, NULL \ casting a user supplied argument is a bad idea, because it removes the compilers ability for type checks. especially because wrong function pointers are a particularly common error scenario (similar to bug 449565).
Its comes from the corresponding cast in: #define G_IMPLEMENT_INTERFACE(TYPE_IFACE, iface_init) { \ const GInterfaceInfo g_implement_interface_info = { \ (GInterfaceInitFunc) iface_init, NULL, NULL \ }; \ g_type_add_interface_static (g_define_type_id, TYPE_IFACE, &g_implement_interface_info); \ }
Tim, any further comment on this ?
Note that we even have an incorrect example in the docs: * G_DEFINE_DYNAMIC_TYPE_EXTENDED (GtkGadget, * gtk_gadget, * GTK_TYPE_THING, * 0, * G_IMPLEMENT_INTERFACE (TYPE_GIZMO, * gtk_gadget_gizmo_init));
Created attachment 126932 [details] [review] [PATCH] Bug 508157 – Add G_IMPLEMENT_DYNAMIC_INTERFACE docs/reference/gobject/gobject-sections.txt | 1 + gobject/gtypemodule.h | 27 ++++++++++++++++++++++++--- 2 files changed, 25 insertions(+), 3 deletions(-)
I'd be interested in this, too. The implementation is trivial and it works well. I'm using the same code in several plugins for GIO and others. Not sure about the name though. G_IMPLEMENT_DYNAMIC_INTERFACE() sounds wrong since the interface itself doesn't have to be dynamic. I'd rather name it G_IMPLEMENT_INTERFACE_DYNAMIC() or something like that.
I chose the name in analogy to the G_DEFINE_TYPE/G_DEFINE_DYNAMIC_TYPE pair instead of using the name suggestion from comment 0.
Yeah, but G_DEFINE_DYNAMIC_TYPE actually *defines* a dynamic type. G_IMPLEMENT_DYNAMIC_INTERFACE wouldn't *define* a new dynamic interface. It would just use an existing (possibly static), hence the DYNAMIC_INTERFACE part of the name is slightly misleading. But I don't have a strong opinion on this and I'm not in the position to decide on that anyway, so ... ;)
Yeah, DYNAMIC_INTERFACE seems wrong to me. There is nothing dynamic about the interface itself really.
Created attachment 148374 [details] [review] Add G_IMPLEMENT_INTERFACE_DYNAMIC Convenience macro to easy interface addition for dynamic types.
Updated the patch with the other name and no cast (as per comment #1). Also updated the since: markings in the api docs.
(In reply to comment #3) > Tim, any further comment on this ? Yes, we should really have a test when adding this. (In reply to comment #11) > Updated the patch with the other name and no cast (as per comment #1). > Also updated the since: markings in the api docs. Great, looks good to me. I'd just also like to see a test for it.
I commited this with a test. However, i really think we want the cast there. Typechecking that the type of the iface init function is GInterfaceInitFunc is useless, because that is very generic: typedef void (*GInterfaceInitFunc) (gpointer g_iface, gpointer iface_data); Whereas all actual implementations are specific: static void g_emblem_iface_init (GIconIface *iface); I'd much prefer the second one, avoiding an extra cast to the real interface type in each implementation. Additionally, all other similar macros (G_IMPLEMENT_INTERFACE, G_DEFINE_TYPE*, G_DEFINE_ABSTRACT_TYPE*) already do the casting for all init functions.
Created attachment 148519 [details] [review] Cast iface_init to GInterfaceInitFunc This is in line with what all other type define macros do.
Would like to commit this. Opinions?
Ugh, i accidentally commited this. Revert if you disagree. Sorry about that...
(In reply to comment #15) > Would like to commit this. Opinions? (In reply to comment #16) > Ugh, i accidentally commited this. Revert if you disagree. Sorry about that... Oh, I thought you had already. I can definitely see your point, so no probs.