Bug 7442

Created attachment 6144 [details] [review]
cairomm_refptr.patch

Non-finished possible solution. The examples show that something is wrong with
the refcounting when using this. It always takes me time to get smartpointer
implementations right.

A couple of problems:
First of all, in your castable copy constructor, you don't initialize the
pCppRefcount_ member variable, and there's no way to do it currently because you
can't access the refcount pointer of the other class.  In my quick-and-dirty
hack, I just made this variable public and assigned it, but we probably want an
accessor for this so that we don't need to make it public.

The second issue is that you're no longer calling 'reference()' when you
construct or copy-construct the RefPtr.  Therefore, the underlying (C)
reference-count never gets incremented.  But you're still calling unreference()
in the RefPtr destructor.  You shouldn't call it here and just let the Cpp
object destroy the underlying C object (otherwise you're destroying the cairo
object twice and Cairo complains that cr->ref_count isn't greater than 0)

With these changes, the examples compile and run fine for me.

Created attachment 6168 [details] [review]
Updated per description above

Here's a modified patch with the changes I mentioned above.  For getting the
reference count from RefPtr<T_CastFrom>, I just added a public refcount()
method that I hid from doxygen.  I'm not exactly happy with that solution, but
I couldn't really think of a better one (I thought about trying to use friend
classes, but couldn't achieve what I wanted).

I've also removed the reference() and unreference() methods from all of the
refcounted classes, since they're not used anymore.  

Let me know if it works for you.

hmm, I just noticed that Gtk::PrintContext::get_cairo_context calls
Cairo::Context::reference().  I'm not sure whether PrintContext needs to call it
(after all, that's what the RefPtr is supposed to be for), but it's something to
keep in mind since we'll have to change that if we get rid of the
reference/unreference methods.

Excellent. I'll look this over later.

I had not planned to remove the reference(), unreference() methods, but maybe we
can.

OK, I have commited your patch to RefPtr<> with some small changes. Well done on
spotting the problem. I noticed that I used the same technique to get the
ref-count in a smartpointer that I implemented in Glom.

I have not committed the changes to the other files, so the reference() and
unreference() methods remain. We need them so we that have some generic way to
give the object an extra reference() when we have received the object from a
get_() method, because get methods (unlike create methods) do not give us a
reference.

I was almost tempted to remove the use of RefPtr<> altogether now that we have
made it so simple, but without it I think we'd have to add documentation with
text like "You must delete the returned object when you have finished with it."
and "You will not have ownership of the returned object so you should not delete
it.". By forcing people to delegate memory management to RefPtr, and hiding the
extra references() in our cairomm and gtkmm methods, we remove this worry.

I am tempted to rename it to Cairo::sharedptr<>.

Let's keep an eye on this. It's a big change.

ok.  I'm fine with leaving the reference() and unreference() methods in if
they're needed.  I'm curious about your comment about removing the RefPtr<>
altogether:  could you give an example of what the interface would look like
without the RefPtr?  Would you just return a pointer to a dynamically allocated
object?  I'd prefer to make it more difficult for the user to forget to free
allocated memory, so I still like the smart pointer interface.

regarding a possible rename to sharedptr, I'm open to it.  One request that I
got a while ago from a user was to provide a typedef inside the class so that it
would be easier to change the pointer type without having to change the
application code.  For example if we provided the following type:

class Context
{
  typedef sharedptr<Context> pointer;

  // rest of class
};

Then instead of an application doing the following:
sharedptr<Context> cr = Context::create(...);

they could simply do:
Context::pointer cr = Context::create(...);

That way we could have changed the Cairo::pointer type from RefPtr to sharedptr
(or even move to the new tr1 shared_ptr in the future) and applications wouldn't
have to change anything (unless they used the RefPtr directly).  Do you think it
would be worthwhile to do something like that?  The C++ standard library uses
this idiom a lot (although generally with raw pointers, not smart pointers), so
I don't think it would be too surprising to most people.

> could you give an example of what the interface would look like
without the RefPtr?

We'd have to force the caller to use delete, and explain how that works repeatedly:

/** ...
 * @result A FontFace. You should delete this object when you are finished with
it, and you should do that as soon as possible after you are finished with it.
Note that deleting this object will not necessarily delete the underlying
CairoFontFace.
 */
FontFace* get_font_face();

> One request that I got a while ago from a user was to provide a typedef inside
> the class so that it would be easier to change the pointer type without having
> to change the application code.
[quoting again from later]
> That way we could have changed the Cairo::pointer type from RefPtr to 
> sharedptr (or even move to the new tr1 shared_ptr in the future) and 
> applications wouldn't have to change anything

I guess they wanted to use a regularly-compiled cairomm with, for instance,
SomeOtherSmartpointer<Cairo> instead of RefPtr<Cairo>. Even if you could get
this to compile, there'd be all sorts of linker or runtime problems. You can't
change an ABI just by changing the header for it, and the RefPtr type is in our
ABI. 

Or maybe they wanted to do some custom build of cairomm. That would be very
unusual and it wouldn't be much more work for them to just replace refptr.h.

But maybe I don't understand what they need. They should post to the list,
ideally supplying a patch.

> they could simply do: Context::pointer cr = Context::create(...);

That seems like something else. That's just a syntactic convenience. I
considered it for glibmm/gtkmm as well as again for cairomm. But I think it's a
rather unfamiliar syntax that would probably confuse many people and which would
obscure the fact that they are using a smartpointer.

Something is still not quite right:

murrayc@ubuntumurrayc:~/cvs/gnome216/cairomm/examples/png_file$ valgrind
--num-callers=20 --tool=memcheck .libs/lt-example_png_file
==4956== Memcheck, a memory error detector.
==4956== Copyright (C) 2002-2005, and GNU GPL'd, by Julian Seward et al.
==4956== Using LibVEX rev 1471, a library for dynamic binary translation.
==4956== Copyright (C) 2004-2005, and GNU GPL'd, by OpenWorks LLP.
==4956== Using valgrind-3.1.0-Debian, a dynamic binary instrumentation framework.
==4956== Copyright (C) 2000-2005, and GNU GPL'd, by Julian Seward et al.
==4956== For more details, rerun with: -v
==4956==
Wrote png file "image.png"
==4956== Invalid read of size 4
==4956==    at 0x8049353: main (refptr.h:200)
==4956==  Address 0x45DC938 is 0 bytes inside a block of size 4 free'd
==4956==    at 0x401D268: operator delete(void*) (vg_replace_malloc.c:246)
==4956==    by 0x8049002: main (refptr.h:211)
==4956==
==4956== Invalid write of size 4
==4956==    at 0x8049358: main (refptr.h:200)
==4956==  Address 0x45DC938 is 0 bytes inside a block of size 4 free'd
==4956==    at 0x401D268: operator delete(void*) (vg_replace_malloc.c:246)
==4956==    by 0x8049002: main (refptr.h:211)
==4956==
==4956== Invalid free() / delete / delete[]
==4956==    at 0x401D268: operator delete(void*) (vg_replace_malloc.c:246)
==4956==    by 0x8049365: main (refptr.h:211)
==4956==  Address 0x45DC938 is 0 bytes inside a block of size 4 free'd
==4956==    at 0x401D268: operator delete(void*) (vg_replace_malloc.c:246)
==4956==    by 0x8049002: main (refptr.h:211)
==4956==
==4956== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 43 from 1)
==4956== malloc/free: in use at exit: 0 bytes in 0 blocks.
==4956== malloc/free: 169 allocs, 170 frees, 1,466,619 bytes allocated.
==4956== For counts of detected errors, rerun with: -v
==4956== No malloc'd blocks -- no leaks are possible.

Fixed.

(In reply to comment #8)
> > could you give an example of what the interface would look like
> without the RefPtr?
> 
> We'd have to force the caller to use delete, and explain how that works
repeatedly:
> 
> /** ...
>  * @result A FontFace. You should delete this object when you are finished with
> it, and you should do that as soon as possible after you are finished with it.
> Note that deleting this object will not necessarily delete the underlying
> CairoFontFace.
>  */
> FontFace* get_font_face();

But then we get back to the issue of what it means to 'copy' an object returned
like this don't we?  Does a copy simply increment the reference or does it
create a new object?

> I guess they wanted to use a regularly-compiled cairomm with, for instance,
> SomeOtherSmartpointer<Cairo> instead of RefPtr<Cairo>. Even if you could get
> this to compile, there'd be all sorts of linker or runtime problems. You can't
> change an ABI just by changing the header for it, and the RefPtr type is in our
> ABI. 

As far as I know, the user just preferred that syntax since he used it elsewhere
in his program, and wanted a similar interface in cairomm.  I don't believe his
goal was to use a different kind of smartpointer.  He just pointed out that if
applications used this typedef-ed syntax, then if we (cairomm developers)
decided to change the implementation of the pointer, it would only amount to an
ABI change (meaning his program would only need a recompile) instead of an ABI +
API change (meaning he'd have to change his source code and recompile)

> That seems like something else. That's just a syntactic convenience. I
> considered it for glibmm/gtkmm as well as again for cairomm. But I think it's a
> rather unfamiliar syntax that would probably confuse many people and which would
> obscure the fact that they are using a smartpointer.

Yes, there is that.  I'm honestly pretty ambivalent about it, but it seemed
slightly related, so I thought I'd pass it along.  Of course, by adding a class
typedef, people could still use the standard RefPtr<T> syntax, but those that
wanted a T::pointer syntax could use that.  So there's not much real downside to
adding it.  But I'm straying from the topic of the original bug report...

> > FontFace* get_font_face();
> 
> But then we get back to the issue of what it means to 'copy' an object returned
> like this don't we?  Does a copy simply increment the reference or does it
> create a new object?

Well it just wouldn't have a copy constructor, like it doesn't have one now.

> Of course, by adding a class typedef, people could still use the standard 
> RefPtr<T> syntax, but those that wanted a T::pointer syntax could use that.  
> So there's not much real downside to adding it.

Except that lots of people would think that they were two different ways of
doing things. They'd be afraid to mix them, and you'd have to deal with the
emails from mistaken people that said that they made it work by doing it the
other way, reinforcing people's idea that they are real alternatives. I'd prefer
to keep it simple.

(In reply to comment #12)
> Well it just wouldn't have a copy constructor, like it doesn't have one now.

Fair enough.  I still prefer the managed smartpointer interface though.

> Except that lots of people would think that they were two different ways of
> doing things. They'd be afraid to mix them, and you'd have to deal with the
> emails from mistaken people that said that they made it work by doing it the
> other way, reinforcing people's idea that they are real alternatives. I'd prefer
> to keep it simple.

Also a good point.  I've emailed the person who requested the interface telling
him about this discussion, and invited him to add a comment here or open a new
bug if he still felt strongly about it.