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.

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<Glib::ustring, CppOptionEntry> type_map_entries;
+  using type_map_entries = std::map<Glib::ustring, CppOptionEntry>;
   type_map_entries map_entries_;
 
   GOptionGroup* gobject_;
-  bool has_ownership_; //Whether the gobject_ belongs to this C++ instance.
 #endif //DOXYGEN_SHOULD_SKIP_THIS
 
 private: