GNOME Bugzilla – Bug 743349
goption: Add boxed type for GOptionGroup
Last modified: 2015-02-17 18:38:12 UTC
.
Created attachment 295170 [details] [review] goption: Add boxed type for GOptionGroup This would allow bindings to use _get_option_group() functions, which would then allow them to use GOption parsing.
Review of attachment 295170 [details] [review]: Why not refcounting?
Created attachment 295172 [details] [review] goption: Add boxed type for GOptionGroup This would allow bindings to use _get_option_group() functions, which would then allow them to use GOption parsing.
Created attachment 295189 [details] [review] goption: Add boxed type for GOptionGroup This would allow bindings to use _get_option_group() functions, which would then allow them to use GOption parsing.
Review of attachment 295189 [details] [review]: Just about right -- can you deprecate free() [moving the code to unref] and make the refcounting non-atomic?
Created attachment 295623 [details] [review] goption: Add boxed type for GOptionGroup This would allow bindings to use _get_option_group() functions, which would then allow them to use GOption parsing.
(In reply to comment #5) > Review of attachment 295189 [details] [review]: > > Just about right -- can you deprecate free() [moving the code to unref] and Done. > make the refcounting non-atomic? I don't understand that. Why?
We're trying to move away from our general approach of "make all refcounting atomic" for performance reasons. I don't really think it matters much in this particular case, but as a general pattern, it's something that we are trying to change. I can't imagine any situation in which you would want to use a GOptionGroup from multiple threads at the same time, so as a default, we should make it non-atomic.
Created attachment 295673 [details] [review] goption: Add boxed type for GOptionGroup This would allow bindings to use _get_option_group() functions, which would then allow them to use GOption parsing.
Created attachment 295675 [details] [review] goption: Add boxed type for GOptionGroup This would allow bindings to use _get_option_group() functions, which would then allow them to use GOption parsing.
Review of attachment 295675 [details] [review]: Please update the documentation for g_option_context_add_group() and add a (transfer full) annotation there. Same for g_option_context_set_main_group(). In kind, g_option_context_get_main_group() could use a (transfer none) annotation on its return value. ::: glib/goption.c @@ +2283,3 @@ + * + * Atomically increments the reference count of @group by one. + * This function is MT-safe and may be called from any thread. !! @@ +2306,3 @@ + * If the reference count drops to 0, the @group will be freed. + * and all memory allocated by the hash table is released. + * This function is MT-safe and may be called from any thread. !! ::: gobject/gboxed.c @@ +166,3 @@ G_DEFINE_BOXED_TYPE (GChecksum, g_checksum, g_checksum_copy, g_checksum_free) +G_DEFINE_BOXED_TYPE (GOptionGroup, g_option_group, g_option_group_ref, g_option_group_free) I think you wanted _unref here?
Created attachment 296598 [details] [review] goption: Add boxed type for GOptionGroup This would allow bindings to use _get_option_group() functions, which would then allow them to use GOption parsing. This also adds introspection annotations to g_option_context_add_group(), g_option_context_set_main_group() and g_option_context_get_main_group().
Review of attachment 296598 [details] [review]: Looks good to go, but please address the three minor docs issues before pushing. ::: glib/goption.c @@ +2280,3 @@ + * @group: a #GOptionGroup + * + * Atomically increments the reference count of @group by one. The word "atomic" is still here. It should be removed. @@ +2300,3 @@ + * @group: a #GOptionGroup + * + * Atomically decrements the reference count of @group by one. ditto. @@ +2302,3 @@ + * Atomically decrements the reference count of @group by one. + * If the reference count drops to 0, the @group will be freed. + * and all memory allocated by the hash table is released. this line "and all memory allocated by the hash table is released." looks like it is included by accident.
Created attachment 296603 [details] [review] goption: Add boxed type for GOptionGroup This would allow bindings to use _get_option_group() functions, which would then allow them to use GOption parsing. This also adds introspection annotations to g_option_context_add_group(), g_option_context_set_main_group() and g_option_context_get_main_group().
Attachment 296603 [details] pushed as 43df97a - goption: Add boxed type for GOptionGroup
Shall g_option_group_free be replaced by g_option_group_unref in glib/glib-autocleanups.h? When I try to build glibmm with the latest glib from the git repository, I get the following compile error: /opt/gnome/include/glib-2.0/glib/glib-autocleanups.h: In function ‘void glib_autoptr_cleanup_GOptionGroup(GOptionGroup**)’: /opt/gnome/include/glib-2.0/glib/glib-autocleanups.h:44:45: error: ‘void g_option_group_free(GOptionGroup*)’ is deprecated (declared at /opt/gnome/include/glib-2.0/glib/goption.h:368) [-Werror=deprecated-declarations] G_DEFINE_AUTOPTR_CLEANUP_FUNC(GOptionGroup, g_option_group_free) ^ I don't know what G_DEFINE_AUTOPTR_CLEANUP_FUNC() does, and I don't know if replacing g_option_group_free by g_option_group_unref is the right fix, but it fixes the build error in glibmm.
https://git.gnome.org/browse/glib/commit/?id=619832f729fbe696575fe1c42a3101eab7691427