GNOME Bugzilla – Bug 583399
gtkmm3: add a WeakPtr?
Last modified: 2016-09-30 08:36: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.
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.
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..
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
Created attachment 300784 [details] An implementation of weak reference I've attached my implementation of weak reference -- feedbacks are appreciated...
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.
(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.
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.
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.
(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;
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.
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.
Created attachment 308235 [details] [review] patch: Glib::RefPtr: Enable disallowance with certain classes Updated version.
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.
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.
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.
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.
I've added noexcept to everything in WeakRef, and filed bug 755048 for further discussion of the RefPtr patch.
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.