After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 583399 - gtkmm3: add a WeakPtr?
gtkmm3: add a WeakPtr?
Status: RESOLVED FIXED
Product: glibmm
Classification: Bindings
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkmm-forge
gtkmm-forge
Depends on:
Blocks:
 
 
Reported: 2009-05-21 02:42 UTC by Jonathon Jongsma
Modified: 2016-09-30 08:36 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
An implementation of weak reference (4.01 KB, text/x-chdr)
2015-04-02 02:05 UTC, worknesday
  Details
patch: Add Glib::WeakRef<> (14.94 KB, patch)
2015-05-24 14:07 UTC, Kjell Ahlstedt
none Details | Review
patch: Glib::RefPtr: Enable disallowance with certain classes (2.43 KB, patch)
2015-05-26 14:06 UTC, Kjell Ahlstedt
none Details | Review
patch: Add Glib::WeakRef<> (19.09 KB, patch)
2015-07-27 13:48 UTC, Kjell Ahlstedt
none Details | Review
patch: Glib::RefPtr: Enable disallowance with certain classes (1.74 KB, patch)
2015-07-27 17:23 UTC, Kjell Ahlstedt
none Details | Review
patch: Add Glib::WeakRef<> (20.56 KB, patch)
2015-08-28 14:46 UTC, Kjell Ahlstedt
committed Details | Review

Description Jonathon Jongsma 2009-05-21 02:42:28 UTC
glib has a concept of a weak pointer.  boost also has a weak_ptr associated with shared_ptr.  It would be nice if glibmm provided a WeakPtr to complement RefPtr.
Comment 1 Murray Cumming 2011-02-21 11:37:12 UTC
We could wrap glib's weak references:
http://library.gnome.org/devel/gobject/unstable/gobject-memory.html#gobject-memory-weakref

I'd want some example of how we'd use them though.
Comment 2 apathadeus 2015-03-31 03:53:43 UTC
I think a weak reference is somewhat necessary -- consider this scenario:

You want to connect a slot to a signal from a widget using sigc++. The signal takes no parameters (e.g. Gtk::Button::signal_pressed). However, in the context if your goal, you need to capture/bind parameters (such as the button itself) to this slot.

You can do this with sigc::bind; however, if you bind with the RefPtr, you would create a circular reference, right? Then this widget would never get destroyed unless your process finshes.

Let me know if my concern is legitimate.

I've never really done open source dev -- I'm a newb in this regard.

I have implemented one locally; it integrates with Glib::RefPtr and Glib::ObjectBase. I was wondering if it's something that maintainers would like to take a look..
Comment 3 apathadeus 2015-03-31 04:25:08 UTC
Should I supply my implementation as an attachment of this bug?

In terms of use -- I modelled it after Python's weakref.

I will define it someone loosely here:

It's a template class WeakRef<T>; it can be initialized & assigned from a WeakRef<T> and RefPtr<T>.

The WeakRef has a method called ref() that produces a RefPtr<T>. Should the underlying gobject be alive, then this returned RefPtr<T> is non-null; otherwise, this RefPtr references null.

example:

    WeakRef<Gst::AppSink> w;
    {
        Glib::RefPtr<Gst::AppSink> s = Gst::AppSink::create();
        w = s;

        printf("exist=%d\n",(bool) w.ref()); // exist=1
    }
    printf("exist=%d\n",(bool) w.ref()); // exist=0
Comment 4 worknesday 2015-04-02 02:05:25 UTC
Created attachment 300784 [details]
An implementation of weak reference

I've attached my implementation of weak reference -- feedbacks are appreciated...
Comment 5 Kjell Ahlstedt 2015-04-28 07:34:59 UTC
There are two groups of functions in glib that deal with weak pointers.

1. g_object_add_weak_pointer()
   g_object_remove_weak_pointer()
   g_object_weak_ref()
   g_object_weak_unref()

2. g_weak_ref_init()
   g_weak_ref_set()
   g_weak_ref_get()
   g_weak_ref_clear()

Comment 1 links to a description of group 1. The patch in comment 4 wraps
group 2.

I think it's fine to concentrate on group 2. The pair of classes Glib::RefPtr/
Glib::WeakRef becomes very similar to std::shared_ptr/std::weak_ptr in C++11.

Shall the new class be called WeakRef or WeakPtr? Since it's called GWeakRef
in glib, it's probably fine to stick to WeakRef.

Is it necessary to store T_CppObject* pCppObject_ in WeakRef?

What shall WeakRef<>::ref() be called?
  ref(), like in the patch?
  get(), like the corresponding function in glib?
  lock(), like the corresponding function in std::weak_ptr?

Glib::RefPtr<T> can store a pointer to an object of any type, T, which has
reference() and unreference() member functions. WeakRef<T> is restricted to
classes that derive from Glib::ObjectBase. This restriction is probably
acceptible, but it would be fine if an attempt to use WeakRef for other classes
would result in a compilation error. I don't think that will happen. It will
be a run-time error. How about something like this:

template <typename T_CppObject, bool is_derived_from_ObjectBase =
  sigc::is_base_and_derived<Glib::ObjectBase,T_CppObject>::value>
class WeakRef;

template <typename T_CppObject>
class WeakRef<T_CppObject, true>
{
  ........
};

Don't implement WeakRef<T_CppObject, false>.

If we accept that glibmm requires a C++11 compatible compiler,
sigc::is_base_and_derived can be replaced by std::is_base_of.
Comment 6 Murray Cumming 2015-04-28 08:03:43 UTC
(Thanks, workwednesday. It would be nice to have a real name, mentioned in a commit message, after git adding the new file, please.)

(In reply to Kjell Ahlstedt from comment #5)
> I think it's fine to concentrate on group 2.

OK.

> The pair of classes
> Glib::RefPtr/
> Glib::WeakRef becomes very similar to std::shared_ptr/std::weak_ptr in C++11.
> 
> Shall the new class be called WeakRef or WeakPtr? Since it's called GWeakRef
> in glib, it's probably fine to stick to WeakRef.

Yes.

> Is it necessary to store T_CppObject* pCppObject_ in WeakRef?

Yes, to implement ref/get/ock()? Or would you want to use Glib::wrap() for that?
 
> What shall WeakRef<>::ref() be called?
>   ref(), like in the patch?
>   get(), like the corresponding function in glib?

Yes, please.

>   lock(), like the corresponding function in std::weak_ptr?

That seems like a strange name.

> Glib::RefPtr<T> can store a pointer to an object of any type, T, which has
> reference() and unreference() member functions. WeakRef<T> is restricted to
> classes that derive from Glib::ObjectBase. This restriction is probably
> acceptible, but it would be fine if an attempt to use WeakRef for other
> classes
> would result in a compilation error. I don't think that will happen. It will
> be a run-time error. How about something like this:
> 
> template <typename T_CppObject, bool is_derived_from_ObjectBase =
>   sigc::is_base_and_derived<Glib::ObjectBase,T_CppObject>::value>
> class WeakRef;
> 
> template <typename T_CppObject>
> class WeakRef<T_CppObject, true>
> {
>   ........
> };
> 
> Don't implement WeakRef<T_CppObject, false>.

OK. That seems useful.

I wish we could also stop RefPtr from being used with Widget classes.

> If we accept that glibmm requires a C++11 compatible compiler,
> sigc::is_base_and_derived can be replaced by std::is_base_of.

I guess we would start with #idefing. Please leave a TODO for it for now.

I don't like the use of goto in the code, by the way.

Of course, we'll need some test added too.
Comment 7 worknesday 2015-04-30 03:57:11 UTC
Hi Murray,

In reference to comment 6, you've seem to answered some of the choice question with "yes", so I'm confused :)

> I don't like the use of goto in the code, by the way.

it's just a pattern that I've adopted for dealing with unwinding failures in a function.

Of course, change it up however you see fit; you're the boss ;)

Initially, I was hoping to do this all from git, but was having problems getting glibmm to compile straight out of git. So, I didn't feel confident enough to just submit this as a git patch to you guys. So I just submitted this as an attachment on bugzilla.

So how did I even test this if I couldn't get glibmm to compile? I had written Weakref for an app that uses glibmm and it compiles with my project.
Comment 8 Kjell Ahlstedt 2015-05-24 14:07:25 UTC
Created attachment 303879 [details] [review]
patch: Add Glib::WeakRef<>

Here's an updated version of WeakRef, and a test case.
I've made WeakRef more like RefPtr by adding some stuff.
Comment 9 Kjell Ahlstedt 2015-05-24 14:35:56 UTC
(In reply to Murray Cumming from comment #6)
> I wish we could also stop RefPtr from being used with Widget classes.

Bjarne Stroustrup's "The C++ Programming Language", 4th edition, section 28.4.4
shows interesting tricks with template classes. I think it would be possible
to create a class like

  template <typename T>
  class AllowUseWithGlibRefPtr
  {
    ???????????
    static const bool value = ???????
  };

in such a way that it can be used in RefPtr, either in pre-C++11

  template <typename T_CppObject, bool allow_use =
    AllowUseWithGlibRefPtr<T_CppObject>::value>
  class RefPtr;

  template <typename T_CppObject>
  class RefPtr<T_CppObject, true>
  {
    ......
  };

or better in C++11

  template <typename T_CppObject>
  class RefPtr<T_CppObject>
  {
    static_assert(AllowUseWithGlibRefPtr<T_CppObject>::value,
      "Glib::RefPtr must not be used with this class.");
    ......
  };

In Gtk::Widget (or should it be in Gtk::Object) define something like

  typedef int m_dont_allow_use_in_glib_refptr;
Comment 10 Kjell Ahlstedt 2015-05-26 14:06:19 UTC
Created attachment 304012 [details] [review]
patch: Glib::RefPtr: Enable disallowance with certain classes

This is the version that uses static_assert() and requires a C++11 compiler.
The version with an extra bool template parameter breaks ABI.
Comment 11 Kjell Ahlstedt 2015-07-27 13:48:13 UTC
Created attachment 308219 [details] [review]
patch: Add Glib::WeakRef<>

Here's an updated version of WeakRef, and a test case.
I've made WeakRef even more similar to RefPtr than the patch in comment 8.
Now it includes move constructor and move assignment.
Comment 12 Kjell Ahlstedt 2015-07-27 17:23:44 UTC
Created attachment 308235 [details] [review]
patch: Glib::RefPtr: Enable disallowance with certain classes

Updated version.
Comment 13 Kjell Ahlstedt 2015-07-28 07:33:10 UTC
I tried to make WeakRef look as much as possible like RefPtr. But I see now
that RefPtr is a moving target. Because of bug 752812 and bug 752876 RefPtr
is being changed right now. When RefPtr has settled, I can see if WeakRef
shall be changed similarly.
Comment 14 Kjell Ahlstedt 2015-08-28 14:46:15 UTC
Created attachment 310194 [details] [review]
patch: Add Glib::WeakRef<>

Here's an updated version of WeakRef, and a test case.
I've added template move constructor and move assignment.
Now it's as similar to RefPtr as it shall be, I think. There's no release()
method. WeakRef has nothing to release.
Comment 15 Murray Cumming 2015-09-14 21:00:12 UTC
Please go ahead and push this. If it needs adjustment then I guess we can do that fairly easily as it's all in the header.
Comment 16 Murray Cumming 2015-09-15 07:17:32 UTC
I've pushed it. I'm not sure about the "Glib::RefPtr: Enable disallowance with certain classes" patch - maybe we should put that in new bug.
Comment 17 Kjell Ahlstedt 2015-09-15 09:58:05 UTC
I've added noexcept to everything in WeakRef, and filed bug 755048 for
further discussion of the RefPtr patch.
Comment 18 Murray Cumming 2016-09-30 08:36:28 UTC
I finally got around to using this with sigc::bind() and just wanted to say thank you to all of you for getting this into glibmm.