It would certainly be possible to use std::shared_ptr instead of Glib::RefPtr
and Cairo::Refptr. But it would increase the memory usage.

I think you once mentioned that you're not very fond of the idea to use the
pimpl idiom in all classes, because it would require one extra pointer per
object. std::shared_ptr uses even more memory. The shared_ptr itself contains 2
pointers: one to the object pointed to, and one to a control block. The control
block contains the ref count and usually some other data, such as a pointer to
the deleter and a counter of std::weak_ptr.

I'd like to modify my comment 1. The pimpl idiom, if used everywhere, adds more
than one pointer to most objects. Each base class also needs a pointer.
A typical widget, such as Button, would contain more than 10 pimpl pointers.

Back to std::shared_ptr. I agree that it's a good idea to replace RefPtr by
std::shared_ptr when we can.

I'm not bothered by any memory overhead by using shared_ptr. It doesn't seem unreasonable.

I've tried this in the refptr_as_sharedptr branch for glibmm:
https://git.gnome.org/browse/glibmm/log/?h=refptr_as_sharedptr

It seems fairly straightforward, but maybe someone can see problems with it.

This patch is not ABI-breaking so it could go in regular glibmm now, to make things easier later:
https://git.gnome.org/browse/glibmm/commit/?h=refptr_as_sharedptr&id=fed31be6cd6f12f61c92f0ef65731edbffd80144

This patch actually makes RefPtr be std::shared_ptr<>:
https://git.gnome.org/browse/glibmm/commit/?h=refptr_as_sharedptr&id=3d1fa437250d18c85a96ae18a724edc7e553fb24
We could remove the RefPtr alias altogether, but for now it makes it simpler while adapting the code.

Unfortunately the WeakPtr test still crashes, though I'm not sure why. valgrind suggests that the instances are being deleted when std::shared_ptr<>::get() is called, but that can't be right.

(In reply to Murray Cumming from comment #4)
> Unfortunately the WeakPtr test still crashes, though I'm not sure why.
> valgrind suggests that the instances are being deleted when
> std::shared_ptr<>::get() is called, but that can't be right.

I've fixed it here:
https://git.gnome.org/browse/glibmm/commit/?h=refptr_as_sharedptr&id=5bc5036f49fe2b15f4fc823194db13cdb207cda7

However, we have to protect users by similar mistakes, so user should not be able to create RefPtr instance without specifying the deleter. So we could either write in documentation, that user should use method make_refptr_for_instance or specify a deleter, or instead of doing an alias, extend shared_ptr class.

(In reply to Marcin Kolny (IRC: loganek) from comment #5)
> (In reply to Murray Cumming from comment #4)
> > Unfortunately the WeakPtr test still crashes, though I'm not sure why.
> > valgrind suggests that the instances are being deleted when
> > std::shared_ptr<>::get() is called, but that can't be right.
> 
> I've fixed it here:
> https://git.gnome.org/browse/glibmm/commit/
> ?h=refptr_as_sharedptr&id=5bc5036f49fe2b15f4fc823194db13cdb207cda7

Thanks.

I found most of these because the compiler complained that the class's destructor (as used by shared_ptr<> without a Deleter) was protected. Maybe this only worked in the test because some class doesn't have a protected destructor.

> However, we have to protect users by similar mistakes, so user should not be
> able to create RefPtr instance without specifying the deleter. So we could
> either write in documentation, that user should use method
> make_refptr_for_instance or specify a deleter, or instead of doing an alias,
> extend shared_ptr class.

We should document it yes. Deriving from shared_ptr<> seems awkward and lengthy. Are you suggesting another alternative?

(In reply to Murray Cumming from comment #6)
> We should document it yes. Deriving from shared_ptr<> seems awkward and
> lengthy. Are you suggesting another alternative?
Unfortunately, unlike unique_ptr, where deleter is template parameter, shared_ptr takes a deleter as a constructor's parameter, so we can't define alias. For now I don't see any alternative, but I'll investigate this topic more.

Would it make sense to rebase refptr_as_sharedptr branch to master now?

And ofc. merge this branch to master

Thanks for the reminder. I've rebased it as refptr_as_sharedptr_v2:
https://git.gnome.org/browse/glibmm/log/?h=refptr_as_sharedptr_v2

I'm still worried about the need to use make_refptr_for_instance() so I'd like some other opinions. But I would love to replace RefPtr.

Then we'll have to try it in gtkmm too.

Factory method seems to be the best choice, IMO, so I'm not concerned about going with make_refptr_for_instance() (I might consider another name, but I'm fine with the concept).
I'm not sure about other *mm libraries (however, since they all use the same generator, so I guess it's the same), but in gstreamermm user never creates it's own RefPtr object because: constructors are protected or private, and we always have factory methods for each class.

So user never can do: RefPtr<SomeType> instance(new SomeType()); It just won't compile.

Ofc. maintainers/contributors will have to be very careful, but I don't think it's a problem.

I did some research on the internet [1], and basically, the answer is: factory method - so exactly what we do in *mm anyway.

I'm attaching link to only one source, but it's very common problem, and usually conclusion is the same.

[1] http://stackoverflow.com/questions/27331315/typedef-a-shared-ptr-type-with-a-static-custom-deleter-similar-to-unique-ptr

Is it possible to get a little more elaboration on the pros of this? The opening post mentions 2 drawbacks of Glib::RefPtr that I can see:

 * RefPtr is intrusive
 * RefPtr is not a standard C++ thing

It seems to me that the intrusive nature of Glib::RefPtr is more of a benefit than a problem, because it lets us use the refcount already present in GObject. Having to artificially maintain that at 1 and use a separately stored C++ refcount and maintain all the extra pointers that requires - doesn't really seem worth whatever 'purity' comes with using something from the C++ stdlib instead.

So I'm not sure how these add up to the move being a near necessity. Sure, an ABI break is possible, and the extra RAM use is probably not a practical issue... but this seems to be defending the decision before it's clear why it was made. Are the 2 points above more important than I know, or are there other reasons too?

I'm not doing this only to play devil's advocate, although I admit that's a part of it. :D If this has already been played out in other discussions, please let me know any links.

Thanks for the questions.

Intrusive smart pointers impose a requirement on the objects that they can point to. By using std::shared_ptr<> (non-intrusive) we would not bother application programmers with that detail unnecessarily. gtkmm generally tries to hide uninteresting details of the C API - that was the purpose of RefPtr and now we can make it even better.

We would also mostly avoid the problem of having to tell people not to use RefPtr with widgets. They would be able to use std::shared_ptr<> with widgets too - they already can.

In general, we do consider being as standard-like as possible to be a very good thing. Avoiding unnecessary or arbitrary reimplementation improves our own code quality by having less code that can be wrong, and it makes application code easier to understand by introducing less new concepts.

Not sure I see the purpose of expanding the use of shared_ptr for widgets. 

One of the things I remember struggling with when I first started doing GUI programming was the basic concept that a widget corresponded to a particular place on the screen, determined by its owner, which meant that you could not "share" a widget across different parents without explicit add/remove logic for both of them.

gtkmm already has very nice semantics for lifetime management for widgets, with no real conceptual benefits that I can see from adding shared_ptr style semantics.

so far, RefPtr has been used in the *mm libs to cover objects whose lifetime can't be managed like widgets (typically references to opaque resources whose cost is deliberately obscure). switching to shared_ptr to manage such resources does make a certain kind of sense (although as djb noted above, not total sense). 

but adding shared_ptr to widget seems like something that will (a) confuse users about the existing lifetime management API for widgets (b) provide no real benefit because in general widgets are *not* shared resources - they are created, packed into a parent container and that's the end of their story. in the few cases where this isn't true (widgets that get moved around and re-used across time), gtkmm already provides a very nice way to get this right.

(just spent a week or two working with boost::intrusive for the first time, and sort-of-kind-of loving it)

> Not sure I see the purpose of expanding the use of shared_ptr for widgets. 

I'm not suggesting that it should be used, or that there is a particularly good use for it, though there might be. But people try it and get confused when it doesn't work. At least recent versions of gtkmm have a static_assert() to stop it.

(In reply to Murray Cumming from comment #13)
> Intrusive smart pointers impose a requirement on the objects that they can
> point to.

I think you've been careful to warn in advance that Glib::RefPtr is not a general-purpose smart pointer, so I'd hope users know they must use an appropriate class for other cases.

And again, I think that intrusiveness is actually a significant benefit for the intended usage - pointing to GObjects, which already have a pre-existing refcount. Integrating directly with the framework that's already there makes a lot more sense to me than adding a redundant second layer of reference counting.


> By using std::shared_ptr<> (non-intrusive) we would not bother
> application programmers with that detail unnecessarily.

I've never found RefPtr to be confusing, obstructive, or otherwise bothersome. It's just what the *mm libraries need me to use, so I used it, and that was the end of it. The explanations I later read about how it integrates with the GObject refcount made me understand and appreciate it fully, but that level of understanding wasn't necessary for just getting on with using it.


> gtkmm generally tries to hide uninteresting details of the C API
> - that was the purpose of RefPtr and now we can make it even better.

Indeed, it does, and that's much appreciated. But to me, having to return such objects by shared_ptr is still an uninteresting (and due to the -> operator rather less readable) detail - just with a different label stuck on it. ;-)


> In general, we do consider being as standard-like as possible to be a very
> good thing. Avoiding unnecessary or arbitrary reimplementation improves our
> own code quality by having less code that can be wrong, and it makes
> application code easier to understand by introducing less new concepts.

Once/if a user starts learning more about the GObject basis (which in my case flowed naturally from learning the nicer bindings first), then Glib::RefPtr is arguably more idiomatic than shared_ptr. It kinda depends whether we take the perspective of a C++11 user or a GObject user - and I think we are fated to require a certain blend of both. It's definitely good to abstract away the more hair-raising parts, but it's not all bad, and/or feasible to make opaque.

To extrapolate - take Glib::Variant. Should it be attempted to replace or abstract that with std::variant<About, One, Hundred, Types> - just to say we're 'more C++' - or simply accept that having a specific wrapper that directly maps onto the existing C/GObj base is the most appropriate choice?

I must stress that one of the things I like the most about the *mm libs is how much effort you put into using modern C++, so I'm normally all in favour of anything in that direction. I'm just not totally convinced it's necessary here, i.e that the costs of changing over are outweighed by its benefits.

IOW, I don't think providing custom types is inherently bad, if they're more directly appropriate to what's being wrapped. I think Glib::RefPtr is just that.


To continue the aside, I also find it hard to imagine much use for shared_ptr with widgets - every scenario I think up, it quickly seems better to use one of the recommended forms of memory management.

but then, whether this is good or bad, the only time I have ever needed to use a shared_ptr was because I wanted to use a unique_ptr but had to put it in a lambda being passed to a sigc::slot, which isn't movable, meaning that it couldn't have a member unique_ptr... hence a shared_ptr, which is really only multi-owner for the most brief of moments.

Application developers should not need to concern themselves with the GObject implementation details. I don't see an advantage in making our API more obviously based on those implementation details. We successfully hide GObject/GtkWidget memory management for widgets, making it like normal C++ memory management. It makes sense to make GObject memory management more obviously standard too.

> the only time I have ever needed to use a shared_ptr was because I wanted to
> use a unique_ptr but had to put it in a lambda being passed to a sigc::slot,
> which isn't movable

You might need to use std::weak_ptr to avoid a leaked reference, depending on what you are actually doing. I think that's mentioned here:
https://www.youtube.com/watch?v=JfmTagWcqoE

Let's try to keep lengthy discussions on the mailing list.

I'll try this out after the next tarball release.

I have just done this for Cairo::RefPtr and updated gtkmm appropriately, in their git masters. That seems like a good first test of the idea.

Here is my latest version of glibmm with std::shared_ptr:
https://git.gnome.org/browse/glibmm/log/?h=refptr_as_sharedptr_v3

There are some RefPtr<>(this) remaining, which will not now work as expected:
https://git.gnome.org/browse/glibmm/tree/gio/src/socket.ccg?h=refptr_as_sharedptr_v3#n114
and
https://git.gnome.org/browse/glibmm/tree/glib/src/iochannel.ccg?h=refptr_as_sharedptr_v3#n336

It would be ugly to derive from std::enable_shared_from_this() just for these uses. Maybe we should just, for instance, have a Glib::IOSource::create() that can take a bare Glib::IOSource*.

There is also a RefPtr<>(this) in Gtk::TextMark.
And bug 547306 shows that RefPtr<>(this) can be used in other modules as well.

When you removed RefPtr<>::cast_static() from Gtk::FlowBox::bind_list_store()
and Gtk::ListBox::bind_list_store(), was that because cast_static() is
problematic when RefPtr is replaced by shared_ptr?

I noticed that your fix does not work when I tried to build gtkmm_documentation/
examples/book/listmodel/example.

This is a working alternative without cast_static():
  store ? store->Gio::ListModel::gobj() : nullptr,

I guess that it will work also if store is a shared_ptr.
The test for an empty store is necessary. The called gtk+ functions accept a
nullptr.

The problem with implicit polymorphic casts not working in arguments to template functions, and thus requiring an explicit static_cast, has perplexed me several times.

As for replacing cast_static, I guess this should work:
http://www.cplusplus.com/reference/memory/static_pointer_cast/

Any thoughts about the RefPtr(this) problem in comment #20 ?

Deriving from std::enable_shared_from_this is possible, but I agree that it's
ugly. The only alternative I can see now is to include a std::weak_ptr data
member in Glib::ObjectBase or Glib::Object. I don't know if it would be
difficult to initialize it correctly, and it's only slightly less ugly.

Created attachment 349285 [details] [review]
0001-Socket-Avoid-creating-a-RefPtr-to-this.patch

Created attachment 349286 [details] [review]
0002-IOChannel-Avoid-creating-a-RefPtr-to-this.patch

Do you object to these two patches, to generally avoid the generally distasteful RefPtr(this), regardless of whether we make the bigger changes?

Created attachment 349288 [details] [review]
0001-TextMark-Avoid-creating-a-RefPtr-to-this.patch

By the way, the gtkmm demo seems to run without problems when using this, after rebuilding pangomm, atkmm, and gtkmm.

These three patches are alright with me. I have no objections.

I have pushed them, and this patch to prepare for RefPtr-as-shared_ptr:
https://git.gnome.org/browse/glibmm/commit/?id=305f2b7d2c84c37906b83b591b945915e355dfac

If there are no big objections, I'll push the actual patches to uses shared_ptr<> soon:
https://git.gnome.org/browse/glibmm/log/?h=refptr_as_sharedptr_v4
(starting with "RefPtr: Make this an alias for shared_ptr<> instead.")

I have pushed the changes to make RefPtr be std::shared_ptr. I would consider reverting it if we find serious problems, but I hope it will succeed.

I've already found one issue: When storing std::shared_ptr<T>s in a Gtk::TreeModel, when the T is not a Glib::Object, it now tries to use the Value specialization for RefPtr:

/** Partial specialization for std::shared_ptr<> to Glib::Object.
 * @ingroup glibmmValue
 */
template <class T>
class Value<std::shared_ptr<T>> : public ValueBase_Object
{
public:
  using CppType = std::shared_ptr<T>;
  using CType = typename T::BaseObjectType*;

  static GType value_type() { return T::get_base_type(); }

  void set(const CppType& data) { set_object(data.get()); }
  CppType get() const { return std::dynamic_pointer_cast<T>(get_object_copy()); }
};

We need to make this specialization only apply for std::shared_ptr<T> where T derives from Glib::ObjectBase. I guess we can do that with std::enable_if.

Created attachment 349558 [details] [review]
0001-Value-Only-use-RefPtr-specialization-for-ObjectBase-.patch

This fixes the problem, but I wonder if there's a simple way to do it.

Incidentally, this made the template specializations for Value<Glib::RefPtr<T>> and Value<Glib::RefPtr<const T>> ambiguous, so I had to combine them, using std::remove_const<>. I think I prefer that anyway, but I wonder what caused the ambiguity.

I have committed this fix: https://git.gnome.org/browse/glibmm/commit/?id=0aa8c79f09e407087596158693f3a3098ad6aa5e

It specializes for Value of RefPtr of any type that has get_base_type(), not just any type derived from Glib::ObjectBase.

I'd welcome any suggestion to simplify it. It took me a while to get this working.

I'd also like to remove this special-casing from the Value type, if possible. Maybe putting it in a conversion function somewhere that the user would not see.

More about the shared_ptr to this problem: The kind of workaround I used in comment #28 won't work for this code in Gtk::Builder::get_widget_derived(), because it would mean changing API that's actually used by applications:

Glib::RefPtr<Gtk::Builder> refThis(this);
refThis->reference(); //take a copy.
widget = new T_Widget(pCWidget, refThis, std::forward<Args>(args)...);

from https://git.gnome.org/browse/gtkmm/tree/gtk/src/builder.hg#n548

Any ideas?

I can see two possibilities.
1. Let Builder derive from std::enable_shared_from_this.
2. Add a private std::weak_ptr data member. Let all create*() methods
   assign the newly created shared_ptr to the weak_ptr.
   In get_widget_derived(), get a shared_ptr with weak_ptr.lock().

Created attachment 349962 [details] [review]
0001-Builder-get_widget_derived-Make-this-static.patch

This seems less drastic. What do you think?

You're right. That's an acceptable solution. But why have you removed 'const'
in 'const T_Widget*& widget' in the version that takes a const Builder?
A const Builder should only return pointers to const widgets.

Two more comments:

The doxygen documentation of RefPtr is a bit strange right now. The comment that
starts "/** This would not be useful," becomes the documenation of
make_refptr_for_instance(). RefPtrDeleter should either be documented or
(probably better) hidden from doxygen.

A suppose that you want to avoid RefPtr(this) because you don't want any calls
to reference() for objects in RefPtrs. But there are lots of such calls
elsewhere, in code generated by _WRAP_METHOD(..., refreturn).

(In reply to Kjell Ahlstedt from comment #38)
> You're right. That's an acceptable solution. But why have you removed 'const'
> in 'const T_Widget*& widget' in the version that takes a const Builder?
> A const Builder should only return pointers to const widgets.

Sorry, that was just a copy/paste typo. Thanks for catching it. I will push this patch with that correction.
 
> Two more comments:
> 
> The doxygen documentation of RefPtr is a bit strange right now. The comment
> that
> starts "/** This would not be useful," becomes the documenation of
> make_refptr_for_instance(). RefPtrDeleter should either be documented or
> (probably better) hidden from doxygen.

Thanks. I've improved that.

> A suppose that you want to avoid RefPtr(this) because you don't want any
> calls
> to reference() for objects in RefPtrs.

I actually wanted to avoid it because it was causing use-after-free, but now that I think about it, it should probably work. The two unrelated std::shared_ptr<>s would each have their own single refcount, and the underlying C++ instance should only be deleted when the last refcount is released. I wonder why what wasn't working.

I'd like to avoid it anyway, because it should not generally work with a normal std::shared_ptr, so it seems like bad style.

> But there are lots of such calls
> elsewhere, in code generated by _WRAP_METHOD(..., refreturn).

I have no problem with that in general.

Isn't this bug fixed now?

When Glib::RefPtr became a std::shared_ptr,
  typedef int dont_allow_use_in_glib_refptr_;
in gtkmm/gtk/gtkmm/object.h lost its meaning. Was that deliberate, or at least
a known and acceptable side-effect?

I guess it's just an acceptable side-effect. Feel free to clean it up. Thanks.

The compiler treats Glib::RefPtr and Cairo::RefPtr as the same type, since they
are both defined as

  template <class T_CppObject>
  using RefPtr = std::shared_ptr<T_CppObject>;

This complicates the definition of specializations of templates, such as

namespace Glib {
  template <typename T>
  class Value;

  namespace Container_Helpers {
    template <typename T>
    struct TypeTraits;
  }
}

These templates need different specializations for Glib::RefPtr<T> and
Cairo::RefPtr<T>.

Would it be possible to define Glib::RefPtr and Cairo::RefPtr as subclasses of
std::shared_ptr? Something like:

namespace [Glib or Cairo] {
  template <class T_CppObject>
  class RefPtr : public std::shared_ptr<T_CppObject>
  {
  public:
    RefPtr(T_CppObject* object) : std::shared_ptr(object, &delete_instance) {}

  private:
    void delete_instance(T_CppObject* object)
    {
      if (object)
        object->unreference();
    }
  };
}

No need for a make_refptr_for_instance() function.

It would simplify template specializations. The compiler would treat
  template <typename T>
  struct TypeTraits<Glib::RefPtr<T>>
and
  template <typename T>
  struct TypeTraits<Cairo::RefPtr<T>>
as two legal specializations, instead of one legal specialization + an illegal
redefinition. (At least that's what I think. I haven't tested.)

Is this worth trying, or is it already known to have unacceptable side-effects?

(In reply to Kjell Ahlstedt from comment #43)
> Would it be possible to define Glib::RefPtr and Cairo::RefPtr as subclasses
> of
> std::shared_ptr?

I don't think that would make sense. We want to specialize on the classes, or something about them, not on the smartpointers that they happen to be used with. Maybe we can enable the specialization only if the underlying class has cobj() or gobj(). Maybe, with C++17, we could even put part in a constexpr if.

In fact, we should assume that we will remove these type aliases some time, so we use just std::shared_ptr<> wherever we now use Glib::RefPtr or Cairo::RefPtr.

Would it be easier if cairomm and glibmm both used cobj(), with no gobj()?

> Would it be easier if cairomm and glibmm both used cobj(), with no gobj()?

No, that wouldn't make any difference when it comes to template specializations.
There are many differences between cairomm classes and glibmm/gtkmm classes.
We would still need different specializations. As long as we need
specializations for only a few cairomm classes (right now Region and Surface),
it's no big problem to specify those classes explicitly.

There are also important differences between cairo and glib. For instance,
class Value<Glib::RefPtr<T>> inherits from ValueBase_Object but
class Value<Cairo::RefPtr<T>> inherits from ValueBase_Boxed.

I don't think subclassing std::shared_ptr is inherently forbidden. It's subject to the usual caveats about lacking a virtual destructor and hence not deleting a subclass through a base pointer to it - but would anyone sensible ever want to do that to a smart pointer, anyway?

Still, it's probably discouraged. The general advice is to use composition, not inheritance, i.e. to create classes that contain it and expose required methods through forwarding. If you specifically need to use it as-a shared_ptr anywhere, obviously that won't work - and doing the forwarding is tedious in either case.

It's probably possible to handle all this in the templates that need different behaviour for shared_ptrs to different types, e.g. via std::enable_if and such, but it may not be worth the hassle/maintenance when compared to the other options.