From 8a01f927bb7b1740c2008fc35a16c2b326271428 Mon Sep 17 00:00:00 2001 From: Kjell Ahlstedt Date: Mon, 20 Mar 2017 15:49:15 +0100 Subject: Glib::OptionGroup: Take advantage of GOptionGroup's refcount GOptionGroup is refcounted. Glib::OptionGroup::has_ownership is unnecessary. Replace gobj_give_ownership() by gobj_copy(). Glib::OptionGroup is not made refcounted, for reasons explained in a comment in optiongroup.hg. --- glib/src/optioncontext.ccg | 8 ++++---- glib/src/optiongroup.ccg | 18 ++++++------------ glib/src/optiongroup.hg | 22 +++++++++++++++++----- 3 files changed, 27 insertions(+), 21 deletions(-) diff --git a/glib/src/optioncontext.ccg b/glib/src/optioncontext.ccg index ed05faa2..a6132d0f 100644 --- a/glib/src/optioncontext.ccg +++ b/glib/src/optioncontext.ccg @@ -94,15 +94,15 @@ OptionContext::~OptionContext() void OptionContext::add_group(OptionGroup& group) { - // Strangely, GObjectContext actually takes ownership of the GOptionGroup, deleting it later. - g_option_context_add_group(gobj(), (group).gobj_give_ownership()); + // GObjectContext takes ownership of the GOptionGroup, unrefing it later. + g_option_context_add_group(gobj(), group.gobj_copy()); } void OptionContext::set_main_group(OptionGroup& group) { - // Strangely, GObjectContext actually takes ownership of the GOptionGroup, deleting it later. - g_option_context_set_main_group(gobj(), (group).gobj_give_ownership()); + // GObjectContext takes ownership of the GOptionGroup, unrefing it later. + g_option_context_set_main_group(gobj(), group.gobj_copy()); } /* diff --git a/glib/src/optiongroup.ccg b/glib/src/optiongroup.ccg index 81d924bb..b4e63a8d 100644 --- a/glib/src/optiongroup.ccg +++ b/glib/src/optiongroup.ccg @@ -269,8 +269,7 @@ OptionGroup::option_arg_callback( OptionGroup::OptionGroup(const Glib::ustring& name, const Glib::ustring& description, const Glib::ustring& help_description) : gobject_(g_option_group_new(name.c_str(), description.c_str(), help_description.c_str(), - this /* user_data */, nullptr /* destroy_func */)), - has_ownership_(true) + this /* user_data */, nullptr /* destroy_func */)) { // g_callback_pre_parse(), post_parse_callback(), g_callback_error(), and // option_arg_callback() depend on user_data being this. The first three @@ -283,18 +282,16 @@ OptionGroup::OptionGroup(const Glib::ustring& name, const Glib::ustring& descrip g_option_group_set_error_hook(gobj(), &g_callback_error); } -OptionGroup::OptionGroup(GOptionGroup* castitem) : gobject_(castitem), has_ownership_(true) +OptionGroup::OptionGroup(GOptionGroup* castitem) : gobject_(castitem) { // Always takes ownership - never takes copy. } OptionGroup::OptionGroup(OptionGroup&& other) noexcept : map_entries_(std::move(other.map_entries_)), - gobject_(std::move(other.gobject_)), - has_ownership_(std::move(other.has_ownership_)) + gobject_(std::move(other.gobject_)) { other.gobject_ = nullptr; - other.has_ownership_ = false; } OptionGroup& @@ -304,10 +301,8 @@ OptionGroup::operator=(OptionGroup&& other) noexcept map_entries_ = std::move(other.map_entries_); gobject_ = std::move(other.gobject_); - has_ownership_ = std::move(other.has_ownership_); other.gobject_ = nullptr; - other.has_ownership_ = false; return *this; } @@ -322,7 +317,7 @@ OptionGroup::release_gobject() noexcept cpp_entry.release_c_arg(); } - if (has_ownership_ && gobject_) + if (gobject_) { g_option_group_unref(gobj()); gobject_ = nullptr; @@ -847,10 +842,9 @@ OptionGroup::CppOptionEntry::convert_c_to_cpp() } GOptionGroup* -OptionGroup::gobj_give_ownership() +OptionGroup::gobj_copy() const { - has_ownership_ = false; - return gobj(); + return g_option_group_ref(gobject_); } } // namespace Glib diff --git a/glib/src/optiongroup.hg b/glib/src/optiongroup.hg index 6db4e9e4..e7a7b2e5 100644 --- a/glib/src/optiongroup.hg +++ b/glib/src/optiongroup.hg @@ -37,8 +37,16 @@ class OptionEntry; class OptionContext; #endif //DOXYGEN_SHOULD_SKIP_THIS -//TODO: GOptionGroup is now refcounted. See https://bugzilla.gnome.org/show_bug.cgi?id=743349 -//When we can break API/ABI, make Glib::OptionGroup refcounted. _CLASS_OPAQUE_REFCOUNTED? +// GOptionGroup is now refcounted. See https://bugzilla.gnome.org/show_bug.cgi?id=743349 +// But Glib::OptionGroup can't be a _CLASS_OPAQUE_REFCOUNTED, because it has +// other data members than gobject_. The memory management would be more involved +// if Glib::OptionGroup were refcounted. It would need a separate refcount for the +// wrapper, like in Glib::IOChannel and Glib::Source. It's easier and good enough +// to let Glib::OptionGroup remain non-refcounted. GOptionGroup's refcount still helps. +// E.g. when GOptionContext takes ownership of a GOptionGroup, it's given a ref. +// A drawback with this approach is that it's possible to have more than one wrapper +// for one GOptionGroup, each wrapper with its own ref. + /** An OptionGroup defines the options in a single group. * Libraries which need to parse commandline options are expected to provide a function that allows their OptionGroups to * be added to the application's OptionContext. @@ -167,9 +175,14 @@ public: _WRAP_METHOD(void set_translation_domain(const Glib::ustring& domain), g_option_group_set_translation_domain) + /// Provides access to the underlying C instance. GOptionGroup* gobj() { return gobject_; } + + /// Provides access to the underlying C instance. const GOptionGroup* gobj() const { return gobject_; } - GOptionGroup* gobj_give_ownership(); + + /// Provides access to the underlying C instance. The caller is responsible for unrefing it. + GOptionGroup* gobj_copy() const; protected: @@ -201,11 +214,10 @@ protected: gpointer data, GError** error); //Map of entry names to CppOptionEntry: - typedef std::map type_map_entries; + using type_map_entries = std::map; type_map_entries map_entries_; GOptionGroup* gobject_; - bool has_ownership_; //Whether the gobject_ belongs to this C++ instance. #endif //DOXYGEN_SHOULD_SKIP_THIS private: -- cgit v1.2.1