GNOME Bugzilla – Bug 449565
Add G_DEFINE_BOXED_TYPE()
Last modified: 2011-03-22 14:24:04 UTC
There are over 10 functions in Pango very similar to this: GType pango_font_metrics_get_type (void) { static GType our_type = 0; if (our_type == 0) our_type = g_boxed_type_register_static (I_("PangoFontMetrics"), (GBoxedCopyFunc)pango_font_metrics_ref, (GBoxedFreeFunc)pango_font_metrics_unref); return our_type; } I just went over all of them and added G_UNLIKELY to them all. (from my reading of gboxed.c, g_boxed_type_register_static() is not idempotent. At least it's not obvious that it is. So if we may need to change the idiom later, etc, it helps to have a macro we can adjust.)
g_boxed_type_register_static() has g_return_val_if_fail (g_type_from_name (name) == 0, 0); which makes it a programming error to call it twice.
i'm not sure i understand exactly what is proposed here, can you please attach a patch if this worth to be kept open?
#define G_DEFINE_BOXED_TYPE(TypeName, type_name, _copyfunc_, _freefunc_) \ GType \ type_name##_get_type (void) \ { \ static GType our_type = 0; \ \ /* TODO: make this thread-safe */ \ if (G_UNLIKELY (our_type == 0)) \ our_type = g_boxed_type_register_static (I_(TypeName), \ (GBoxedCopyFunc)_copyfunc_, \ (GBoxedFreeFunc)_freefunc_); \ \ return our_type; \ }
A more complete set of macros can be found in that file: http://svn.gnome.org/viewcvs/libegg/trunk/libegg/util/egg-macros.h?revision=819&view=markup It also introduces G_DEFINE_QUARK (actually EGG_DEFINE_QUARK). Guess having G_DEFINE_QUARK could improve code quality of glib applications: At least I find myself to use quarks, as I do not want to write the (short) boilerplate code needed to introduce some quark.
(In reply to comment #4) > A more complete set of macros can be found in that file: > > http://svn.gnome.org/viewcvs/libegg/trunk/libegg/util/egg-macros.h?revision=819&view=markup > > It also introduces G_DEFINE_QUARK (actually EGG_DEFINE_QUARK). Guess having > G_DEFINE_QUARK could improve code quality of glib applications: At least I find > myself to use quarks, as I do not want to write the (short) boilerplate code > needed to introduce some quark. you should really make your _get_type (and quark registration) functions thread-safe in there. take a look at initializer1() in http://svn.gnome.org/viewcvs/glib/trunk/tests/onceinit.c?view=markup for a simple example.
(In reply to comment #5) Thread safety applied: http://svn.gnome.org/viewcvs/libegg/trunk/libegg/util/egg-macros.h?r1=823&r2=825 Shall I prepare a patch to move those macros into glib?
(In reply to comment #6) > (In reply to comment #5) > > Thread safety applied: > http://svn.gnome.org/viewcvs/libegg/trunk/libegg/util/egg-macros.h?r1=823&r2=825 > > Shall I prepare a patch to move those macros into glib? well, for G_DEFINE_BOXED_TYPE() that might make sense. i'm not so sure about your quark thingy though, haven't really seen the need for that yet. also, take your time on this one, since we're currently API frozen.
(In reply to comment #7) > i'm not so sure about your quark thingy though, haven't really seen the need for that yet. Well, you need quarks to define custom error domains and often enough I voted against GError based error reporting because I was to lazy to paste the required custom_error_quark function. Maybe a problem of mine. Don't know.
I find the quark macro useful too. Specially if using the quark in more than one function...
Created attachment 101601 [details] [review] add G_DEFINE_[BOXED|POINTER]_TYPE, generating threadsafe get_type functions; and use them in gobject
At GUADEC Tim commented on my code that the cast to GBoxedCopyFunc and GBoxedFreeFunc should be left to the user. So, that's one place to fix in the patch. Other than that, looks good to me, and definitely an improvement over current practice.
If the copy and free funcs are public (which they usually will be), they'll take a Foo* not a gpointer, so you'll _almost always_ have to put the cast in the G_DEFINE_BOXED_TYPE. That strikes me as rather inconvenient...
Created attachment 102318 [details] [review] updated patch; document the new macros It would be really great if this made it into 2.16 ...
Comment on attachment 102318 [details] [review] updated patch; document the new macros >+/* convenience macros for boxed type registration: >+ * macro arguments: TypeName, copy_func, free_func >+ * example: G_DEFINE_BOXED_TYPE (GtkFoo, gtk_foo, gtk_foo_copy, gtk_foo_free) Since this is a convenience macro, wouldn't it be more convenient to implicitly form the copy/free function names (type_name##_copy, similar to type_name##_class_init) and in case their naming is different provide G_DEFINE_BOXED_TYPE_WITH_FUNCTIONS() where they can be explicitly given?
(In reply to comment #14) > (From update of attachment 102318 [details] [review] [edit]) > >+/* convenience macros for boxed type registration: > >+ * macro arguments: TypeName, copy_func, free_func > >+ * example: G_DEFINE_BOXED_TYPE (GtkFoo, gtk_foo, gtk_foo_copy, gtk_foo_free) > > Since this is a convenience macro, wouldn't it be more convenient to implicitly > form the copy/free function names (type_name##_copy, similar to > type_name##_class_init) and in case their naming is different provide > G_DEFINE_BOXED_TYPE_WITH_FUNCTIONS() where they can be explicitly given? Half of types use foo_ref, half use foo_copy, and you do use those functions in real code, so the names must be meaningful and not auto-generated (unlike type_name##_class_init which you don't care about).
Any news on this? Would be nice to have this in 2.18 at least...
Another thing, apart from G_DEFINE_{POINTER,BOXED}_TYPE and G_DEFINE_QUARK would be G_DEFINE_ENUM_TYPE. #define G_DEFINE_ENUM_TYPE(TypeName,type_name,values) \ GType \ type_name##_get_type (void) \ { \ static volatile gsize g_define_type_id__volatile = 0; \ if (g_once_init_enter (&g_define_type_id__volatile)) \ { \ static const GEnumValue v[] = { values, {0, NULL, NULL} }; GType g_define_type_id = \ g_enum_register_static (g_intern_static_string (#TypeName), \ v); \ g_once_init_leave (&g_define_type_id__volatile, g_define_type_id); \ } \ return g_define_type_id__volatile; \ } /* closes type_name##_get_type() */
if we need G_DEFINE_ENUM_TYPE, we also need G_DEFINE_FLAGS_TYPE. And support for both should also be added to glib-mkenums too if we need that
Created attachment 111863 [details] [review] [PATCH] Add G_DEFINE_{BOXED,POINTER}_TYPE[_WITH_CODE]. Bug #449565. docs/reference/gobject/gobject-sections.txt | 4 ++ docs/reference/gobject/tmpl/gtype.sgml | 54 +++++++++++++++++++++++++++ gobject/gtype.h | 37 ++++++++++++++++++ 3 files changed, 95 insertions(+), 0 deletions(-)
In the updated patch, I added _WITH_CODE variants like for we have for G_DEFINE_TYPE.
Created attachment 113209 [details] [review] [PATCH] Add G_DEFINE_{BOXED,POINTER}_TYPE[_WITH_CODE]. Bug #449565. docs/reference/gobject/gobject-sections.txt | 4 + gobject/gtype.h | 82 +++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 0 deletions(-)
This seems like a useful feature, the patch looks about right, can someone who has the expertise review the patch?
Tim? This fixes real issues in almost every gobject-using code. Lets do a QUARK one too. The more places we can make thread-safe initializations automatic the better.
(In reply to comment #12) > If the copy and free funcs are public (which they usually will be), they'll > take a Foo* not a gpointer, so you'll _almost always_ have to put the cast in > the G_DEFINE_BOXED_TYPE. That strikes me as rather inconvenient... And G_DEFINE_TYPE does the casts for you on the init and class_init functions, so it's consistent to have G_DEFINE_BOXED_TYPE do it here too.
Tim, any final comment on this ?
*ping* Any news here? Tim?
Can we also get a G_DEFINE_INTERFACE[_WITH_CODE] please? And yet another ping.
Ping!
Anyone want to prepare more patches, for INTERFACE, ENUM, FLAGS, and QUARK?
INTERFACE is bug 320482
Created attachment 148838 [details] [review] updated patch to master
Ping? Any chance this could make it in for 2.24 ?
(In reply to comment #12) > If the copy and free funcs are public (which they usually will be), they'll > take a Foo* not a gpointer, so you'll _almost always_ have to put the cast in > the G_DEFINE_BOXED_TYPE. That strikes me as rather inconvenient... Good point.(In reply to comment #14) > Since this is a convenience macro, wouldn't it be more convenient to implicitly > form the copy/free function names (type_name##_copy, similar to > type_name##_class_init) and in case their naming is different provide > G_DEFINE_BOXED_TYPE_WITH_FUNCTIONS() where they can be explicitly given? As has been pointed out, _copy and _ref are both in use for these functions, so canonical naming won't work here. We can however still have the compiler check the function _type_, this works by casting the _register function so that it accepts function pointers of a particular boxed struct type. The technique to implement this is e.g. demonstrated in g_test_add(), which takes a type as argument (Fixture) and a function using this type (ftest).
You mean like this? #define _G_DEFINE_BOXED_TYPE_BEGIN(TypeName, type_name, copy_func, free_func) \ GType \ type_name##_get_type (void) \ { \ static volatile gsize g_define_type_id__volatile = 0; \ if (g_once_init_enter (&g_define_type_id__volatile)) \ { \ GType (* _g_register_boxed) (const gchar *, TypeName * (*) (TypeName *), void (*) (TypeName *)) = \ (GType (*) (const gchar *, TypeName * (*) (TypeName *), void (*) (TypeName *))) g_boxed_type_register_static; \ GType g_define_type_id = \ _g_register_boxed (g_intern_static_string (#TypeName), copy_func, free_func); \ { /* custom code follows */ I tried this and it works. However, there are additional problems: * constness. E.g. when using this to replace the hand-written register funcs in gboxed.c: G_DEFINE_BOXED_TYPE (GValueArray, g_value_array, g_value_array_copy, g_value_array_free) but: GValueArray* g_value_array_copy(const GValueArray *value_array); and G_DEFINE_BOXED_TYPE (GVariantType, g_variant_type, g_variant_type_copy, g_variant_type_free) but: GVariantType *g_variant_type_copy (const GVariantType *type); so you get a compiler warning. * TypeName* != C type name, e.g. for G_DEFINE_BOXED_TYPE (GStrv, g_strv, g_strdupv, g_strfreev) where GStrv is gchar**, and g_strdupv/g_strfreev of course don't take GStrv*. This seems a rare problem, but would still be nice to handle it... * renames. E.g. to get the existing type names for gvarianttype and gvariant's get_type functions, I had to do this: #define g_variant_type_get_type g_variant_type_get_gtype G_DEFINE_BOXED_TYPE (GVariantType, g_variant_type, g_variant_type_copy, g_variant_type_free) #undef g_variant_type_get_type #define g_variant_get_type g_variant_get_gtype G_DEFINE_BOXED_TYPE (GVariant, g_variant, g_variant_ref, g_variant_unref) #undef g_variant_get_type I guess that's the best we can do here, or should we make the name of the get_type function a param of the macro too?
Created attachment 158039 [details] [review] updated patches
(In reply to comment #35) > However, there are additional problems: > > * constness. E.g. when using this to replace the hand-written register funcs in > gboxed.c: G_DEFINE_INTERFACE ended up making some assumptions that didn't work cleanly with gio's pre-existing interface type definitions, so there's precedent here.
(In reply to comment #35) > You mean like this? > > #define _G_DEFINE_BOXED_TYPE_BEGIN(TypeName, type_name, copy_func, free_func) \ > GType \ > type_name##_get_type (void) \ > { \ > static volatile gsize g_define_type_id__volatile = 0; \ > if (g_once_init_enter (&g_define_type_id__volatile)) \ > { \ > GType (* _g_register_boxed) (const gchar *, TypeName * (*) (TypeName *), The variable declaration isn't really neccessary, function casts can be inlined, e.g.: ( (void(*)(void)) some_func ) (); Your version is arguably more readable though. > void (*) (TypeName *)) = \ > (GType (*) (const gchar *, TypeName * (*) (TypeName *), void (*) Using "TypeName* (*) (const TypeName*)" would really be the correct approach here and fix your const issues, it'd force const correctness on boxed type implementors though. > * renames. E.g. to get the existing type names for gvarianttype and gvariant's > get_type functions, I had to do this: > > #define g_variant_type_get_type g_variant_type_get_gtype > G_DEFINE_BOXED_TYPE (GVariantType, g_variant_type, g_variant_type_copy, > g_variant_type_free) > #undef g_variant_type_get_type > #define g_variant_get_type g_variant_get_gtype > G_DEFINE_BOXED_TYPE (GVariant, g_variant, g_variant_ref, g_variant_unref) > #undef g_variant_get_type > > I guess that's the best we can do here, or should we make the name of the > get_type function a param of the macro too? I'd really like to avoid this, we also introduced similar macros for object types in order to *enforce* consistent function naming, could Ryan maybe comment here why the inconsistencies are needed?
(In reply to comment #38) > > void (*) (TypeName *)) = \ > > (GType (*) (const gchar *, TypeName * (*) (TypeName *), void (*) > > Using "TypeName* (*) (const TypeName*)" would really be the correct approach > here and fix your const issues, it'd force const correctness on boxed type > implementors though. But the copy_func isn't necessarily const, e.g. it will change the refcount for refcounted boxed types (GHashTable, GArray, GPtrArray,GClosure...).
(In reply to comment #39) > But the copy_func isn't necessarily const, e.g. it will change the refcount for > refcounted boxed types (GHashTable, GArray, GPtrArray,GClosure...). const can mean whatever the API developer decides it should mean. It shouldn't just mean that no bits will changed. For instance, an internal cache could change. gtkmm lets const objects be refed and unrefed (via its smartpointer) because anything else would make the const useless. Well, that's the practice in C++, where const is fully used. But const just doesn't work in C, mostly because there are no const member functions and no mutable.
(In reply to comment #38) > (In reply to comment #35) > > void (*) (TypeName *)) = \ > > (GType (*) (const gchar *, TypeName * (*) (TypeName *), void (*) > > Using "TypeName* (*) (const TypeName*)" would really be the correct approach > here and fix your const issues, it'd force const correctness on boxed type > implementors though. GBoxedCopyFunc uses gpointer not gconstpointer, so leaving the const off would be consistent. To proceed, should I: * add |const|, and use explicit casts when using G_DEFINE_BOXED_TYPE with existing non-const copy funcs, or * leave const off, and use explicit casts when using it with existing const copy funcs ? (The former would introduce more casts than the latter in the uses of this macro in gobject/gio.) > > * renames. E.g. to get the existing type names for gvarianttype and gvariant's > > get_type functions, I had to do this: > > > > #define g_variant_type_get_type g_variant_type_get_gtype > > G_DEFINE_BOXED_TYPE (GVariantType, g_variant_type, g_variant_type_copy, > > g_variant_type_free) > > #undef g_variant_type_get_type > > #define g_variant_get_type g_variant_get_gtype > > G_DEFINE_BOXED_TYPE (GVariant, g_variant, g_variant_ref, g_variant_unref) > > #undef g_variant_get_type > > > > I guess that's the best we can do here, or should we make the name of the > > get_type function a param of the macro too? > > I'd really like to avoid this, we also introduced similar macros for object > types in order to *enforce* consistent function naming, could Ryan maybe > comment here why the inconsistencies are needed? Whatever the reason was, this is now set in stone since it's part of stable 2.24.
i suggest using transparent union types where available and discarding type safety elsewhere. see bug 581023
Can we just get it in already?
Created attachment 165042 [details] [review] Add G_DEFINE_{BOXED,POINTER}_TYPE[_WITH_CODE] Add convenience type definition macros for boxed and pointer types similar to G_DEFINE_TYPE for object types. Bug #449565.
Created attachment 165043 [details] [review] Use G_DEFINE_[BOXED|POINTER]_TYPE instead of handwritten code Now that we have convenience macros to implement boxed and pointer types, use them.
Lets make sure these macros are "static"able, and document them so.
The latest patch in attachment 165042 [details] [review] fixes the const problem (comment 38 ff), by allowing to pass either a Foo * (*) (Foo *) Foo * (*) (const Foo *) GBoxedCopyFunc as the copy func. Anything else will trigger a compiler warning. (Implemented using transparent unions, so only on gcc >= 2.7; on other compilers it will just cast to GBoxedCopyFunc). Are this patch (and the follow-up to make use of it in glib) now ok to commit?
Ping, again?
Got ok-to-commit from #gtk-devel meeting. Pushed to master.
I filed the G_DEFINE_QUARK request (comment 4) as bug 627240, and the G_DEFINE_[ENUM|FLAGS]_TYPE request (comment 17) as bug 627241.
While glib/GTK+ usually compile just fine with -pedantic (which Clang uses by default), this change seems to introduce warnings. One of these warnings (there are many more of these of course), Clang emits: stock_browser.c:62:1: warning: incompatible pointer types initializing 'GType (gchar const *, GBoxedCopyFunc, GBoxedFreeFunc)', expected 'GType (*)(gchar const *, union <anonymous>, union <anonymous>)' [-pedantic] G_DEFINE_BOXED_TYPE (StockItemInfo, stock_item_info, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ gcc -pedantic gives: stock_browser.c: In function 'stock_item_info_get_type': stock_browser.c:62: warning: ISO C prohibits argument conversion to union type stock_browser.c:62: warning: ISO C prohibits argument conversion to union type Is this something that should and can be fixed? Or is -pedantic too strict for glib/GTK+'s liking?
Fwiw, this causes a warning when using clang default compile flags
Created attachment 184042 [details] Test case When compiled using clang, this test case produces this warning: ./clang.c:30:1: warning: incompatible pointer types initializing 'GType (*)(gchar const *, union <anonymous at ./clang.c:30:1>, union <anonymous at ./clang.c:30:1>)' with an expression of type 'GType (gchar const *, GBoxedCopyFunc, GBoxedFreeFunc)' G_DEFINE_BOXED_TYPE(BoxedString, boxed_string, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from ./clang.c:5: In file included from /usr/include/glib-2.0/glib-object.h:25: In file included from /usr/include/glib-2.0/gobject/gbinding.h:31: In file included from /usr/include/glib-2.0/gobject/gobject.h:26: /usr/include/glib-2.0/gobject/gtype.h:1521:72: note: instantiated from: ...copy_func, free_func) G_DEFINE_BOXED_TYPE_WITH_CODE (TypeName, ... ^ /usr/include/glib-2.0/gobject/gtype.h:1538:87: note: instantiated from: ...free_func, _C_) _G_DEFINE_BOXED_TYPE_BEGIN (TypeName, type_name, ... ^ /usr/include/glib-2.0/gobject/gtype.h:1548:16: note: instantiated from: GType (* _g_register_boxed) \ ^ 1 warning generated. clang defines __GNU__ and __GNU_MINOR__ to 4 and 2 so it uses the #define with the union for G_DEFINE_BOXED_TYPE_BEGIN
I guess we could change the definition to use #if (__GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ >= 7)) && !defined(__STRICT_ANSI__) which works since if you pass -pedantic you most likely will also use -ansi (see gcc manual for -pedantic).
This doesn't avoid the clang warning though.
Is there a clang-specific define we could check for to distinguish it from gcc ?
According to google, clang -dM -E - < /dev/null dumps all preprocessor constants defined by clang. On my box, I have: #define __clang__ 1 #define __clang_major__ 2 #define __clang_minor__ 8 #define __clang_patchlevel__ 0 #define __clang_version__ "2.8 (branches/release_28)" #define __llvm__ 1 which could be useful here.