GNOME Bugzilla – Bug 605976
add g_type_ensure(), to ensure that a type has been registered
Last modified: 2012-05-15 20:23:49 UTC
Because lots of _get_type() functions are tagged G_GNUC_CONST, you can't just say "foo_get_type();" to ensure that the type is registered, because it might get optimized out. Currently the standard idiom for working around this is: volatile GType type; type = g_inet_address_get_type (); And as long as you put the assignment on a separate line from the declaration, gcc won't warn that it's a dead store. But clang does. Also, it looks dumb. I'm attaching a patch that adds: #define g_type_ensure(g_type) G_STMT_START { if (G_UNLIKELY ((g_type) == 0)) g_type_init (); } G_STMT_END (The use of "g_type_init()" in the "if" body is arbitrary; the "if" will always be false, but the compiler doesn't know that.) This avoids the dead store warnings, but adds a little bit of dead code. Other possibilities would be: - use a static or extern volatile GType, so that the compiler can't prove that the store is dead - make g_type_ensure() an actual function that just does nothing Or something else? Even if we keep the current idiom it seems like having a macro for it would be nice, and then we can just say "fine, go ahead, everyone should make their _get_type() functions G_GNUC_PURE even though it's wrong"
Created attachment 150751 [details] [review] Add an (internal) g_networking_init() macro rather than explicitly calling g_inet_address_get_type() everywhere
Created attachment 150752 [details] [review] Add g_type_ensure() and use it rather than playing games with volatile This version also avoids the clang "dead assignment" warnings that the previous idiom caused.
Created attachment 179236 [details] [review] Add an (internal) g_networking_init() macro rather than explicitly calling g_inet_address_get_type() everywhere
Created attachment 179237 [details] [review] Add g_type_ensure() and use it rather than playing games with volatile Rebased and updated... gcc 4.6 (which just landed in Rawhide) now warns about the old "volatile GType type = blah_get_type()" idiom as well, so this (or something like it) would be good to get in.
Review of attachment 179237 [details] [review]: ::: gobject/gtype.h @@ +1277,3 @@ + * Since: 2.28 + */ +#define g_type_ensure(g_type) G_STMT_START { if (G_UNLIKELY ((g_type) == (GType)-1)) g_error ("can't happen"); } G_STMT_END Having constant strings in a macro is a bit ugly since they end up separately in each compilation unit (though the linker should coalesce them, so this isn't a real concern). Maybe change the string though to "g_type_ensure failed" so it's more clear where the string came from? Though why not just make this an actual function (that internally does nothing), rather than a macro?
(In reply to comment #0) > > - make g_type_ensure() an actual function that just does nothing Ah, you do suggest this here. Yeah, I think i'd prefer the function version - it gives us a lot more flexibility in the future, versus a situation where we've baked in the macro expansion into various components and apps but later need to change something.
Created attachment 214138 [details] [review] Add g_type_ensure() and use it rather than playing games with volatile ==== rebased and updated to use a function() (and removed the g_networking_init() part, which will happen later) g_type_ensure() ended up not being completely empty, to protect against optimizations of it within libgobject. Another possibility would be to have a separate G_GNUC_INTERNAL g_type_ensure_internal(), and #define g_type_ensure g_type_ensure_internal when GOBJECT_COMPILATION is set, and then only put the code in the internal version. But the code is only going to add one or two assembly language instructions to the, so it's not a huge deal to have it there always...
Review of attachment 214138 [details] [review]: Looks like a nice cleanup. One question: Can we use this for gio/gdbusprivate.c:ensure_required_types() too?
Attachment 214138 [details] pushed as e011d2c - Add g_type_ensure() and use it rather than playing games with volatile > One question: Can we use this for gio/gdbusprivate.c:ensure_required_types() > too? I'm pretty sure that code can just go away; it was only needed because one of the threads was intentionally blocking while holding the type lock, which you fixed in bug 651650.