GNOME Bugzilla – Bug 752876
Add missing move assignment operator and move constructor to RefPtr class
Last modified: 2015-09-09 08:24:25 UTC
I wanted to use move constructor in following code: Glib::RefPtr<Gst::Pipeline> p = Gst::Pipeline::create(); Glib::RefPtr<Gst::Element> e = std::move(p); // Gst::Pipeline is derived from Gst::Element I've seen, that move constructor have been added to refptr class, so I expected that above code should work. Unfortunately, it doesn't, because refptr doesn't contain template <class T_CastFrom> inline RefPtr(RefPtr<T_CastFrom>&&) constructor. I've added both move constructor and move assignment operator, which allow to pass different, but castable type as an argument. As I said in bug report https://bugzilla.gnome.org/show_bug.cgi?id=752812 , I've mixed feelings about code src->reference(); src.reset(); but if you don't want to change interface, it's IMO the only possibility to release refptr. I didn't use std::move() and swap() in operator= implementation because of two reasons: 1) I'm not sure if we want to add #include <memory> header 2) Comment in method operator=(RefPtr&& src) says, that it "seems less efficient" Ad) 1 - is it a problem to add this include to refptr.h? Ad) 2 - in fact, it is less efficient, but the overhead is almost imperceptible. Moreover, guys from libstdc++ don't care about it, and use swap in assignment operator's code. If you don't mind, I could make my code cleaner by using move and swap functions.
Created attachment 308162 [details] [review] proposed solution
(In reply to Marcin Kolny (IRC: loganek) from comment #0) > I've mixed feelings about code > src->reference(); > src.reset(); > but if you don't want to change interface, it's IMO the only possibility to > release refptr. Could you keep that discussion in that bug report, please. It seem unrelated.
I'm a little concerned about the use of rvalue references on templated functions ("universal references"), after reading in "Effective Modern C++" how they can take precedence over everything else, but maybe it's less of a problem when we take RefPtr<T_CastFrom> rather than T_CastFrom. I need to take a longer look at it and maybe add some test code to make sure that we call the constructors that we expect to call at various times.
(In reply to Marcin Kolny (IRC: loganek) from comment #0) > I didn't use std::move() and swap() in operator= implementation because of > two reasons: > 1) I'm not sure if we want to add #include <memory> header > 2) Comment in method operator=(RefPtr&& src) says, that it "seems less > efficient" > Ad) 1 - is it a problem to add this include to refptr.h? I think that would be fine, if it's necessary. > Ad) 2 - in fact, it is less efficient, but the overhead is almost > imperceptible. Moreover, guys from libstdc++ don't care about it, and use > swap in assignment operator's code. > If you don't mind, I could make my code cleaner by using move and swap > functions. Actually, libstdc++ doesn't seem to use the swap technique: shared_ptr<> copy constructor: https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libstdc%2B%2B-v3/include/bits/shared_ptr.h;h=f96c07835b625b60d32fb96a6d901a0aa299589a;hb=HEAD#l225 shared_ptr<> assignment operator: https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libstdc%2B%2B-v3/include/bits/shared_ptr.h;h=f96c07835b625b60d32fb96a6d901a0aa299589a;hb=HEAD#l291 Maybe we should stop using the swap technique so we can really make it worthwhile to use the move constructor. But we should do that first before adding another constructor. I've added a test case that's meant to show that things are working as expected. Please feel free to correct it or add to it: https://git.gnome.org/browse/glibmm/commit/?id=a2195549d6477bccfd6365388997d2d5e1752032
Created attachment 308206 [details] [review] proposed solution v2 (with tests) I prepared some tests for my changes. As you can see, I commented a line: g_assert_cmpint(refSomething->max_ref_count(), ==, 1); I'm not sure, what should I do with this condition: * leave commented, and wait for bug 752812 's solution, * uncomment, and let the test fail for now, * completely remove this condition.
Thanks, and sorry. I do now see why you need API to implement this in RefPtr<>. RefPtr<Base> can't access RefPtr<RefPtr>. Please go ahead and try this with a release() method. Do you have any idea why my existing test for the regular move constuctor is causing an extra ref: static void test_refptr_with_parent_move_constructor() { Parent parent(get_something()); g_assert(parent.was_constructed_via_move_constructor()); g_assert(!parent.was_constructed_via_copy_constructor()); g_assert_cmpint(parent.something_ref_count(), ==, 2); //1 in parent and 1 inside get_something() g_assert_cmpint(parent.something_max_ref_count(), ==, 2); //Fails because it is 3 } That constructor seems to be fine in code like this, based on your code: static void test_refptr_universal_reference_move_constructor() { Glib::RefPtr<Something> refSomethingDerived(new Something()); //NOT DERIVED. Glib::RefPtr<Something> refSomething = std::move(refSomethingDerived); g_assert_cmpint(refSomething->ref_count(), ==, 1); g_assert(!refSomethingDerived); // todo see https://bugzilla.gnome.org/show_bug.cgi?id=752812 g_assert_cmpint(refSomething->max_ref_count(), ==, 1); }
(In reply to Murray Cumming from comment #6) > Thanks, and sorry. I do now see why you need API to implement this in > RefPtr<>. RefPtr<Base> can't access RefPtr<RefPtr>. Please go ahead and try > this with a release() method. Does it mean, that I can push my patch posted in bug 752812 ? > > Do you have any idea why my existing test for the regular move constuctor is > causing an extra ref: I think I know. A bug is in Parent constructor: explicit Parent(const Glib::RefPtr<Something>&& something) : something_(std::move(something) Type of std::move return value is const Glib::RefPtr<Something>&&, so for constructing something_, copy constructor (RefPtr(const RefPtr&)) will be used, and not RefPtr(RefPtr&&), as you probably expected to be used. If you want to fix it, just remove 'const' keyword in Parent's move constructor.
(In reply to Marcin Kolny (IRC: loganek) from comment #7) > (In reply to Murray Cumming from comment #6) > > Thanks, and sorry. I do now see why you need API to implement this in > > RefPtr<>. RefPtr<Base> can't access RefPtr<RefPtr>. Please go ahead and try > > this with a release() method. > Does it mean, that I can push my patch posted in bug 752812 ? Yes, if that's what you need. When that's all in master we can take another look and see if anything can be improved even more. Thanks. [snip] > If you want to fix it, just remove 'const' keyword in Parent's move > constructor. Many thanks. I should have seen that. I'll fix that now.
From comment 0: > I didn't use std::move() and swap() in operator= implementation because of two reasons: > 1) I'm not sure if we want to add #include <memory> header Both std::move() and std::swap() are declared in <utility>. From comment 3: > I'm a little concerned about the use of rvalue references on templated functions std::shared_ptr has such a constructor and assignment. template< class Y > shared_ptr( shared_ptr<Y>&& r ); template< class Y > shared_ptr& operator=( shared_ptr<Y>&& r ); From comment 6: > I do now see why you need API to implement this in RefPtr<>. RefPtr<Base> > can't access RefPtr<RefPtr>. (s/RefPtr<RefPtr>/RefPtr<Derived>/, I suppose.) Actually that's possible, if you add // Let all instantiations of RefPtr access private data. template <typename T_CastFrom> friend class RefPtr;
Kjell, I'd welcome any improvements. If we can remove the need for release(), that would be nice too. Then we could consider that issue separately.
Created attachment 308290 [details] [review] 0001-RefPtr-move-assignment-operator-Use-swap.patch Incidentally, I also wonder why using swap() in operator=(&&) causes an extra ref/unref though swap() shouldn't actually cause that. For instance, with this patch, the test fails like so: $ ./glibmm_refptr/test ** ERROR:glibmm_refptr/main.cc:215:void test_refptr_move_assignment_operator(): assertion failed (refSomething2->ref_count() == 1): (2 == 1) Aborted (core dumped)
Created attachment 308291 [details] [review] 0001-RefPtr-move-assignment-operator-Use-swap2.patch Replying to myself, it does work if I use std::move() there, though I don't quite understand why. operator=(const RefPtr<T_CastFrom>& src) should probably have a std::move() too.
Sorry, ignore that last part about operator=(). I thought that was && rather than &.
(In reply to Kjell Ahlstedt from comment #9) > Actually that's possible, if you add > > // Let all instantiations of RefPtr access private data. > template <typename T_CastFrom> > friend class RefPtr; (In reply to Murray Cumming from comment #10) > Kjell, I'd welcome any improvements. If we can remove the need for > release(), that would be nice too. Then we could consider that issue > separately. If we decide to keep release() method in RefPtr class, we won't have to declare friendship between all RefPtr's variants. IMO it's not an improvement, if we have release().
(In reply to Murray Cumming from comment #11) > Incidentally, I also wonder why using swap() in operator=(&&) causes an > extra ref/unref though swap() shouldn't actually cause that. For instance, > with this patch, the test fails like so: Are you familiar with reference collapsing rules? That's why at the beginning of operator=() copy constructor, but not move constructor is called. To avoid it, you should use std::move or std::forward function (in this case, std::move is better).
(In reply to Marcin Kolny (IRC: loganek) from comment #14) > If we decide to keep release() method in RefPtr class, we won't have to > declare friendship between all RefPtr's variants. IMO it's not an > improvement, if we have release(). I agree that release() fits very well in the move constructor and move assignment, but with template <typename T_CastFrom> friend class RefPtr; we can replace src.operator->() by src.pCppObject_ in other methods, if we like.
(In reply to Marcin Kolny (IRC: loganek) from comment #15) > (In reply to Murray Cumming from comment #11) > > Incidentally, I also wonder why using swap() in operator=(&&) causes an > > extra ref/unref though swap() shouldn't actually cause that. For instance, > > with this patch, the test fails like so: > Are you familiar with reference collapsing rules? That's why at the > beginning of operator=() copy constructor, but not move constructor is > called. To avoid it, you should use std::move or std::forward function (in > this case, std::move is better). Ah, yes, this must be the thing about the parameter's type being an rvalue reference, but the parameter being an lvalue (because we can get its address) which can be converted to an rvalue by std::move(), which does a static_cast<RefPtr&&>(src). That will take me some time to get used to. Thanks.
Created attachment 308356 [details] [review] 0001-RefPtr-Remove-release-using-a-templated-friend-state.patch This patch uses a friend statement as suggested by Kjell, removing the release() method. I guess the templated friend statement didn't work with compilers when we first wrote RefPtr, or we tried to avoid it by using operator->().
Created attachment 308357 [details] [review] 0001-RefPtr-Experiment-Remove-non-casting-copy-move-const.patch I also tried this experiment, wondering if we even needed the non-templated constructors and assignment operators. The build is then fine but the test fails: $ ./tests/glibmm_refptr/test ** ERROR:glibmm_refptr/main.cc:123:void test_refptr_copy_constructor(): assertion failed (refSomething->ref_count() == 2): (1 == 2) Aborted (core dumped)
(In reply to Murray Cumming from comment #19) > ERROR:glibmm_refptr/main.cc:123:void test_refptr_copy_constructor(): > assertion failed (refSomething->ref_count() == 2): (1 == 2) > Aborted (core dumped) I guess this is an issue of overload resolution. The move constructor must be taking precedence over the copy constructor, passing a & to && instead of to const &.
> I also tried this experiment, wondering if we even needed the non-templated > constructors and assignment operators. The build is then fine but the test > fails: It is, again, because of reference collapsing rules. Please look at the post: http://stackoverflow.com/questions/14100171/template-constructor-taking-precedence-over-normal-copy-and-move-constructor and the answer: http://stackoverflow.com/a/14100271/1058673
Created attachment 308437 [details] Test case and output from it BS x.y.z = Bjarne Stroustrup: "The C++ Programming Language", 4th edition, section x.y.z Reference collapse is destribed in BS 7.7.3. The somewhat unintuitive rules for template argument deduction is destribed in BS 23.5.2.1: "An lvalue of type X is deduced as an X& and an rvalue as an X." I wonder if these rules are relevant to the RefPtr case. They apply to cases like template <typename T> whatever f(T& t) template <typename T> whatever f(T&& t) In RefPtr it's more like template <typename T> whatever f(const RefPtr<T>& t) template <typename T> whatever f(RefPtr<T>&& t) The type of the function argument is not a reference to a plain template argument. And as long as non-template copy and move constructors and assignment operators are declared, the expected version is selected, contrary to the example linked to in comment 21. I made a test case to see what happens in a case more similar to RefPtr. My conclusion is that if non-template copy constructor and copy assignment operator are not declared in the class, the compiler supplies default ones, which just copy the pointer in the class. Note in the output from ref_ptr3 that none of its members is called in one object construction and one assignment. I'm quite certain that compiler-generated ones are called. ref_ptr2, which has non-template copy operators but no non-template move operators, behaves better. The compiler does not generate move operators when there are user-declared copy operators or destructor. This is described in BS 17.6.
Isn't this bug fixed? Can it be closed?
Yes. Thanks.
I'm concerned that this, or some other move operation, may be causing this bug: https://bugzilla.gnome.org/show_bug.cgi?id=754763