Created attachment 308058 [details] [review]
proposed solution

(In reply to Marcin Kolny (IRC: loganek) from comment #0)
> It's quite ugly solution, moreover, it can leads to unexpected behavior - we
> can't be sure, what actually *_ref method does (I know, usually it only
> increases refcount). 

When has this ever been an actual problem? Does reference() have some side-effect somewhere in gstreamer ?

(In reply to Murray Cumming from comment #2)
> When has this ever been an actual problem? Does reference() have some
> side-effect somewhere in gstreamer ?
No, I've never seen reference method having side-effects, but it's not assured.

Sorry, I'm not prepared to confuse the RefPtr<> API by adding this if it doesn't solve a real problem.

It seems OK to me to make the referencing explicit. These odd and unusual C functions want to take a reference, so that's what you give them. They don't usually really need you to no longer have any references yourself.

Please consider following example:

  Glib::RefPtr<Gst::Buffer> ref_ptr = Gst::Buffer::create();

  // Thread 1:
  1. GstBuffer* raw_ptr = ref_ptr->gobj();
  2. ref_ptr->reference();
  3. ref_ptr.reset();
  4. g_some_function_with_transfer_full (raw_ptr);

  // Thread 2:
  5. GstBuffer *buffer = gst_buffer_make_writable(ref_ptr->gobj());
  6. // some operations on buffer

gst_buffer_make_writable method checks, if refcount equals 1, and if not, it creates a copy of an argument (https://developer.gnome.org/gstreamer/stable/gstreamer-GstBuffer.html#gst-buffer-make-writable).
Imagine, that application calls the lines in a following order: 1, 2, 5, 3, ... so gst_buffer_make_writable will make unnecessary copy of buffer. 

Moreover, according to bug: https://bugzilla.gnome.org/show_bug.cgi?id=752876, I've prepared a test:
  Glib::RefPtr<Something> something = Glib::RefPtr<SomethingDerived>(new SomethingDerived());
  g_assert_cmpint(something->max_ref_count(), ==, 1);
which should definitely pass if we have move constructors. Without "release" method, we're not able to write move constructor:
  template <class T_CastFrom> RefPtr(RefPtr<T_CastFrom>&& src);
and assignment operator properly.
I see your point; RefPtr is core class in glibmm library and we shouldn't add methods which are not really necessary. But "release" method IMO can't be replaced by any set of refptr's operations(ref_ptr->reference();ref_ptr.reset(); might cause side-effects).

Thanks. Yes, I remember seeing something like that awful behaviour in gstreamer before. That's a terrible API. Luckily, it's the only time I've seen such a thing. Even in the best case, I don't see how C++ application code can easily make sure that the refcount of the underlying object is only 1. Hopefully you can hide this behaviour from users of the C++ API. 

By the way, is there any documentation for that function that mentions the refcount needing to be 1? It's not mentioned in the documentation that you linked to:
https://developer.gnome.org/gstreamer/stable/gstreamer-GstBuffer.html#gst-buffer-make-writable
or here:
http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/gstreamer-GstMiniObject.html#gst-mini-object-make-writable

I suggest that we revisit this when we have figured out what move constructors we will have in RefPtr and made sure that they work properly.

(In reply to Murray Cumming from comment #6)
> By the way, is there any documentation for that function that mentions the
> refcount needing to be 1? It's not mentioned in the documentation that you
> linked to:
> https://developer.gnome.org/gstreamer/stable/gstreamer-GstBuffer.html#gst-
> buffer-make-writable
> or here:
> http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/
> gstreamer-GstMiniObject.html#gst-mini-object-make-writable
It's not mentioned in doc for *_make_writable methods, but in documentation of gst_mini_object_is_writable(http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/gstreamer-GstMiniObject.html#gst-mini-object-is-writable) you can find out, what "writable object" actually means. 
> 
> I suggest that we revisit this when we have figured out what move
> constructors we will have in RefPtr and made sure that they work properly.
OK, I'll try to write a tests for move constructor/assignment operator containing universal reference slightly different(not using max_ref_count). We can back here after we close a bug 752876.

Is it safe to use __attribute__((warn_unused_result))? Will all supported
compilers either understand or ignore it?

When an __attribute__ is used in glib, it's used via a preprocessor macro, like

  #if    __GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 4)
  #define G_GNUC_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
  #else
  #define G_GNUC_WARN_UNUSED_RESULT
  #endif /* __GNUC__ */

in glib/glib/gmacros.h.

Sorry, I must have missed that. I would prefer to avoid using that for now. We could discuss that separately. If we use it later, I would definitely prefer to use the glib macro.

(In reply to Kjell Ahlstedt from comment #8)
> Is it safe to use __attribute__((warn_unused_result))? Will all supported
> compilers either understand or ignore it?
You're right. E.g. msvc doesn't support this attribute (it has _Check_return_ instead) https://msdn.microsoft.com/en-us/library/jj159529.aspx, but I'm not sure if they ignore it. Anyway, I agree, we should use glib macro instead (if we keep this attribute at all)

You only plan to use release() in the implementation of a gstreamermm method, right? Could you please show me a whole method that would use it, including the method signature and its implementation.

(In reply to Murray Cumming from comment #11)
> You only plan to use release() in the implementation of a gstreamermm
> method, right? 
Not really, we could use it everywhere, for transfer-full arguments:

  template <class T> inline
  typename T::BaseObjectType* unwrap(Glib::RefPtr<T>&& o)
  {
    return o.release();
  }

And conversion definition:

  _CONVERSION(`Glib::RefPtr<Glib::SomeObject>&&',`GSomeObject*',`Glib::unwrap(std::move($3))')

Could you please give me an example how the actual generated (or hand-written) code would look?

I think it would be unwise to create an unwrap() specialization that uses this special API. A simple call to release() would be less confusing.

(In reply to Murray Cumming from comment #13)
> Could you please give me an example how the actual generated (or
> hand-written) code would look?

Sure, please consider e.g. gst_pad_push method. Declaration in .hg file:
_WRAP_METHOD(Gst::FlowReturn push(Glib::RefPtr<Gst::Buffer>&& buffer), gst_pad_push)

And output .cc file:
Gst::FlowReturn Pad::push(Glib::RefPtr<Gst::Buffer>&& buffer)
{
  return static_cast<Gst::FlowReturn>(gst_pad_push(buffer.release()->gobj()));
}

So, this wouldn't compile?
  auto buffer = create_some_buffer(someargs);
  pad.push(buffer);
But this would?
  pad.push(create_some_buffer());
and this would?

Is there any way to hide the strangeness from the API? For instance:
  create_some_buffer_and_add_to_pad(someargs, pad);


If we knew that the created buffer would always be given to something with full ownership then I guess we could wrap the buffer in a unique_ptr<> somehow, but I doubt the gsreamer API is that clear.

(In reply to Murray Cumming from comment #15)
> So, this wouldn't compile?
>   auto buffer = create_some_buffer(someargs);
>   pad.push(buffer);
> But this would?
>   pad.push(create_some_buffer());
> and this would?
First should work as well, if you add std::move(buffer) instead of buffer. 
> 
> Is there any way to hide the strangeness from the API? For instance:
>   create_some_buffer_and_add_to_pad(someargs, pad);
Not really.. sometimes we can have a buffer created somewhere else. Moreover, in gstreamer there is really a lot of functions (not only in Pad class, and not only with GstBuffer arguments) with transfer-full annotation. 
> 
> 
> If we knew that the created buffer would always be given to something with
> full ownership then I guess we could wrap the buffer in a unique_ptr<>
> somehow, but I doubt the gsreamer API is that clear.
AFAIK, there's no assumption about only-full-ownership buffer's transfer.

OK. Kjell, do you see any alternative to adding RefPtr::release() to make this possible?

No, I don't see any alternative.

If we add RefPtr::release(), it should have a warning in its documentation,
like Glib::ObjectBase::[un]reference().

Marcin wants to have RefPtr release the ownership of its managed object
without affecting the ref count of the object.

The code in comment 0 comes close. Many objects have a gobj_copy() method,
and then the code can be simplified:

  auto raw_ptr = ref_ptr->gobj_copy();
  ref_ptr.reset();
  g_some_function_with_transfer_full (raw_ptr);

Both versions of the code come close to what Marcin wants, but not close enough,
as explained in comment 5.

std::unique_ptr has a release() method, but std::shared_ptr has none.
There is an important difference between std::shared_ptr and Glib::RefPtr.
shared_ptr manages the ref counter itself. An unmanaged object would not have a
ref counter. RefPtr manages an object that contains its own ref counter.

Created attachment 308558 [details] [review]
minor improvements

OK, I added a warning in comment, and replaced warn_unused_result attribute with Glib macro. Please review it. If everything is ok, I'll push it and close this report.

I didn't mean that you should copy the warning verbatim from reference(), rather
write something suitable in the same spirit.

  Most users should not use release(). It can spoil the automatic destruction
  of the managed object. A legitimate use is if you immediately give RefPtr's
  reference to another object.

With this or a similar change I think it's ok, but give Murray a chance to
comment before the push.

If RefPtr uses G_GNUC_WARN_UNUSED_RESULT, refptr.h must #include <glib.h>.

I guess that's not the end of the world.

Marcin, are you waiting for more comments before you make the final update of
the release() method?

(In reply to Kjell Ahlstedt from comment #23)
> Marcin, are you waiting for more comments before you make the final update of
> the release() method?

No, sorry. I was a little bit busy last time, I'm updating it in a few days.

Created attachment 308889 [details] [review]
minor improvements v2

I fixed my patch, by adding glib.h include directive, and using Kjell's comment. 
If you have no objections, I'll push it to the upstream.

It's ok with me.

OK, I've pushed my patch.