Re: [gtkmm] Glib::RefPtr construction

[Michael Johnson <michaelj maine rr com>]

Murray Cumming wrote:
> On Wed, 2003-01-08 at 01:50, Michael Johnson wrote:
> > Murray Cumming wrote:
> > > On Tue, 2003-01-07 at 22:08, Michael Johnson wrote:
> > >   
> > > > In reading through the source for Glib::RefPtr I note that the constructor
> > > >
> > > >     RefPtr<T_CppObject>::RefPtr(T_CppObject* pCppObject)
> > > >
> > > > Does not increase the reference count of the referenced object,
> > > >       
> > >     
> >   
>
> As I said before, that's because GTK objects are created with a
> reference count of 1. If we increase it to 2 then the object will not be
> deleted during the RefPtr<>'s final destructor. But that's the whole
> point of RefPtr<>
What I am proposing, which I guess did not come through clearly, is that
gtkmm objects should initialize their reference count to zero, not to one.

It appears, from reading further on, that the reference counting of gtkmm
objects is being treated as being synonymous with the reference counting
of the GTK+ objects that they are wrapping. To me, this seems wrong. The
gtkmm object and the GTK+ object it is wrapping are not in fact the same
entity. One is merely a proxy for the other. It would be better for the gtkmm
object to be reference counted in its own right, and for the reference count
of the GTK+ object to be incremented only when the gtkmm wrapper acquires
a reference to it, and decremented only when the gtkmm wrapper releases a
reference to it.

In other words, reference counting of gtkmm objects ought to be disconnected
from reference counting of wrapped GTK+ objects. In this way, reference counting
of a gtkmm object can safely be used to manage the lifetime of that object.
By proxy, the reference count of the GTK+ object will be appropriately incremented
and decremented as a reference to it is acquired and released by the gtkmm
object that wraps it. Multiple references to a gtkmm wrapper object are not
multiple references to the GTK+ object; they are multiple references to an
object that holds a *single* reference to the GTK+ object.

One consequence of this paradigm shift would be that the destroy_notify_callback
in Glib::ObjectBase would no longer result in deletion of the wrapper. What
it would do is clear the Glib::ObjectBase pointer to the gobj. Normally,
this should not be an issue, because the gobj should not be destroyed until
the destructor of the gtkmm wrapper releases its reference to the gobj. If
the gobj is ever destroyed before the gtkmm wrapper is destroyed, then something
is wrong and a warning should be issued.

Since it appears that GTK+ uses COM-style reference counting protocol (i.e.
anything that passes a pointer to an object to some other entity increases
the reference count before handing out the pointer) it really only becomes
necessary for Glib::ObjectBase to decrease the gobj reference count at the
appropriate time. However, I'd want to verify this assumption before writing
code based on it.
> > > >  though 
> > > > all other means of assigning a value to a RefPtr (including the 
> > > > assignment operator that takes a pointer)
> > > >  do increase the reference 
> > > > count. Is this intentional, or is it an accidental omission?
> > > >       
> > >     
> >   
>
> Yes, maybe that operator=(T_CppObject*) should be the same as the
> constructor. I'm not sure when that operator= is used anyway. Maybe it's
> safe to change it.
Actually, I was suggesting that the constructor should be changed to be the
same as the operator.
> > > The pCppObject is normally created with a refcount of 1, and the final
> > > RefPtr<> destructor unrefs it to 0, causing pCppObject to be deleted.
> > >
> > >   
> > >     
> > I realize I am a newcomer to the list, and I do not wish to step on
> > any toes, so please consider the following discussion in the spirit in
> > which it is offered, as constructive discourse. In my professional
> > capacity, I have in the past implemented a large, complex system of
> > classes that used smart-pointer management of reference-counted
> > objects and as a result I have learned some things from practical
> > experience.
> >
> > In my opinion, the behavior described above appears to be dangerous
> > and error-prone. Especially in light of the fact that SigC::Object,
> > which is perfectly capable of being managed by RefPtr, does not behave
> > this way.
> >   
>
> RefPtr<> is not intended for this purpose. RefPtr is for use with
> Glib::Object-derived objects only. If you can add some code to prevent
> the template from compiling with anything else then I would welcome the
> patch.
The documentation does not make this distinction. It says that RefPtr is
a reference-counting smart pointer, and that it will work with any class
that has reference() and unreference() methods, *such as* a Glib::ObjectBase-derived
class. As such, it should behave in ways that a general reference-counting
pointer would be expected to behave. This is, I think, as it should be.
> SigC::Ptr might be more useful for your purposes. I haven't used it. I
> normally suggest that people use a more general smartpointer for more
> general needs.
>
> >  It starts with a reference count of zero. Consider the following
> > samples of code:
> >
> >     Glib::RefPtr<Foo> rpFoo;
> >     return (rpFoo = new Foo());
> >
> > Versus:
> >
> >     return Glib::RefPtr<Foo>(new Foo());
> >
> > or even:
> >
> >     Glib::RefPtr<Foo> rpFoo = new Foo();
> >     return rpFoo;
> >
> > These three code examples, which one would reasonably expect to behave
> > the same, in fact yield different results. The first case will result
> > in a reference count that is one higher than the other two cases. This
> > is non-intuitive.
> >   
>
> This is a repetition of your first point about operator=(), isn't it?
Basically, yes. I was trying to illustrate that the behaviors implemented
by the various means of assigning a value to a RefPtr yield inconsistent
results, which is non-intuitive.
> > It also seems to me that constructing the object with a reference
> > count of 1 is conceptually incorrect.
> >   
>
> Well that's GTK+, not gtkmm. That's just the start of the GTK+
> reference-counting problems, all of which are simplified by the gtkmm
> API.
See my comment above regarding disconnection of gtkmm and GTK+ reference
counting. By disconnecting the gtkmm reference counting from the GTK+ reference
counting, you (we) can avoid propagating this dangerous design flaw.
> >  At the time of construction, no "reference" to the object exists,
> > therefore the reference count should be zero. There will not in fact
> > be a reference to the object until a RefPtr (which embodies the
> > reference) is constructed with or assigned a pointer to the object.
> > There is also the question of appropriate assignment of roles and
> > responsibilities. A reference-counted object has a reference count,
> > but does not itself manage that reference count. It merely provides a
> > means for other objects (which embody references) to manage the
> > reference count. It is the role of the reference class to manage the
> > reference count of objects being referenced.
> >   
>
> Are you complaining that GObject holds the reference instead of the
> RefPtr<>? Well, that's how it is. If you can provide a patch that makes
> RefPtr<> more useful then I would be interested to see it.
No, not at all. The overloading of the term "object" here is leading to confusion,
I think. I'm not referring to the GTK+ object reference being held by the
GObject. I am referring to references to the GObject itself, keeping in mind
that references to the GObject are not references to the GTK+ object.

I would be happy to put together a patch that demonstrates how I think RefPtr
and the classes in gtkmm should behave. Perhaps that would more clearly illustrate
what I'm trying to express.
> >  It makes sense then that any method of the reference class that
> > changes the value of the reference should participate in the
> > management of the reference count.
> >
> > Looking at Glib::ObjectBase, there is probably no reason why its
> > overrides of reference() and unreference() could not in turn call the
> > SigC::ObjectBase reference() and unreference() methods. By default,
> > those methods will not delete the object when the reference count goes
> > to zero.
> >   
>
> What would they do? Increase a reference count? What use would the 2nd
> reference count number do?
This is how I would disconnect the GTK+ reference count from the gtkmm reference
count. All classes derived from Glib::ObjectBase already have a reference
counter that is separate from the wrapped GTK+ object's reference count.
It is built into the SigC::ObjectBase class. I am merely suggesting that
Glib::ObjectBase should be exploiting the behavior of SigC::ObjectBase, not
throwing it away.

In fact, it should not be necessary for there to be more than one implementation
of smart pointers in the class hierarchy of gtkmm and sigc. If the SigC::Ptr
were well implemented (and it's not, it depends too much on the underlying
implementation of reference counting in SigC::ObjectBase) then it ought to
be perfectly reasonable and natural for Glib::RefPtr to be a renaming of
SigC::Ptr.
> >  This only happens if the set_manage() method is called, so it is
> > normally safe to construct objects derived from Glib::ObjectBase on
> > the stack. It then becomes simpler for classes derived from
> > Glib::ObjectBase to enable lifetime management by reference counting.
> > They don't have to override reference() and unreference(), they merely
> > need to invoke set_manage() in their constructor.
> >   
>
> This is not the purpose of set_manage() as far as I know.
If the purpose of set_manage() is not to enable lifetime management by reference
counting, then I don't know what it would be for, because that is exactly
what it does.
> >  Nor do they need to (redundantly, because it is already being done by
> > SigC::ObjectBase) include a reference-count data member.
> >
> > This has advantages. Consider the possibility of reference-counted
> > objects being shared across multiple threads. Overlapping calls to the
> > reference() and unreference() methods from different threads must be
> > guarded against. The simplest way to do this is to use an atomic
> > counter so that increment or decrement of the counter is thread-safe.
> > If many classes in gtkmm re-implement reference counting, then all of
> > those classes must be changed in order to make gtkmm thread-safe. If
> > those same classes used the underlying reference counting in
> > SigC::ObjectBase then only SigC::ObjectBase would need to be made
> > thread-safe in order for all classes derived from it to be in turn
> > thread-safe.
> >
> > Does anyone else have any thoughts on this?
> >   
>
> There seems to be 1 possible bug here and a great deal of talk. I would
> be interested in 
> a) Investigation of the effects of changing that operator=().
> b) A patch to implement your ideas, which you have tested.
I'll get to work on a patch. I suspect it will take me some time, because
I need to carefully investigate which classes in gtkmm expect to have their
lifetime managed by reference-counting (i.e. are always allocated on the
heap with new) and which ones basically ignore reference counting (and can
safely be allocated on the stack, such as widgets). The latter classes should
probably have their reference() and unreference() methods turned into protected
or private methods, so that RefPtr cannot be used with them.

..mj