GNOME Bugzilla – Bug 756962
Move operators in Glib::ObjectBase and its subclasses
Last modified: 2015-10-31 08:31:11 UTC
I doubt that all move constructors and move assignment operators in glibmm are correct. CC = copy constructor CA = copy assignment operator MC = move constructor MA = move assignment operator !! = need more thought and/or fix - ObjectBase CC, CA deleted Protected MC, MA !!MC: I suppose that this constructor is meant to always be called before Object::MC. But those two don't work well together. Object::MC calls ObjectBase::initialize_move(), which just prints an error message, if gobject_ is not nullptr. _move_current_wrapper() will not be called. I guess it would be better to let ObjectBase::MC set gobject_ = nullptr, like the other ObjectBase constructors do. !!MA: src.gobject_ is not set to nullptr. Shouldn't _move_current_wrapper() be called? The call to unreference() is dangerous. If it removes the last reference to *gobject_, the *gobject_ object will be deleted, triggering deletion of *this. The pointer to the wrapper should be removed from *gobject_ (with g_object_steal_qdata()) before it's unreferenced. - Object CC, CA deleted Public MC, MA !!MC: Should call sigc::trackable(std::move(src)). MA is ok. - Interface CC, CA deleted Public MC, MA !!MC: Should call sigc::trackable(std::move(src)). !!MA: Should not call ObjectBase::operator=(std::move(src)). (For the same reason that MC does not call ObjectBase::initialize_move(). ObjectBase::operator=() shall be called only once, and we assume that the most derived class, such as Gtk::Entry, also derives from Glib::Object.) - _CLASS_GOBJECT CC, CA deleted Public MC, MA !!MC: Should call the constructors of the virtual base classes: sigc::trackable(std::move(src)), Glib::ObjectBase(std::move(src)) MA is ok. - _CLASS_INTERFACE CC, CA deleted Public MC, MA !!MC: Should call the constructors of the virtual base classes: sigc::trackable(std::move(src)), Glib::ObjectBase(std::move(src)) MA is ok. Unimportant remark: A better name for ObjectBase::_move_current_wrapper() would be _replace_current_wrapper(). std::move() doesn't do anything useful in assignments of int, double, pointers and the like. E.g. in gobject_ = std::move(src.gobject_); custom_type_name_ = std::move(src.custom_type_name_); cpp_destruction_in_progress_ = std::move(src.custom_type_name_); where gobject_ and custom_type_name_ are pointers, and cpp_destruction_in_progress_ is a bool. The moved-from data won't be modified. Calling the constructors of the virtual base classes are important only for classes that are used for creating objects. Some classes are only intended to be further derived from, like ObjectBase, Object, Interface and _CLASS_INTERFACE. Nonetheless it looks better and safer to include those calls in all move constructors. Why is sigc::trackable a _virtual_ base class? Up until now it has not affected the code, because its default constructor has always been the correct constructor to call. A move constructor shall call trackable's move constructor, and that call must be included in every class that can be the most derived class of an object. Move constructors and move assignments can't be used for objects that are accessed through RefPtr, can they? Consider an example: Glib::RefPtr<Glib::Something> some_object11 = Glib::Something::create(); { Glib::RefPtr<Glib::Something> some_object21 = Glib::Something::create(); Glib::RefPtr<Glib::Something> some_object22 = some_object21; // ref_count=2 some_object11->operator=(std::move(*some_object22.operator->())); // The ref_count of the GSomething object is still 2, but there is only // one RefPtr, some_object11, pointing to it. // some_object21 and some_object22 point to a Glib::Something object with // gobject_ == nullptr. When those RefPtrs are deleted, they will call // unreference(), which will make an illegal access to gobject_. } // If the program has not crashed already, if contains memory leaks. // None of the two created Glib::Something objects will be deleted. It's next to impossible to combine RefPtr with move operators of the pointed-to C++ objects. The reference count is in the C object, i.e. the object that is moved from one C++ wrapper object to another. It would perhaps be possible to get a correct reference count in the moved-to object. But the moved-from C++ object will lose its reference count, while still being referenced from a number of RefPtrs. There is no way to know when that C++ object shall be deleted. A comment on the test cases in glibmm/tests. This may look like an assignment, but it is a construction: DerivedObject derived2 = std::move(derived); To get a call to the move assignment operator: DerivedObject derived2; derived2 = std::move(derived);
I think we can avoid having to call sigc::trackable(std::move(src)) and Glib::ObjectBase(std::move(src)) from the move constructors of all derived classes. The job of ObjectBase's move constructor is really done by Object's move constructor, when it calls ObjectBase::initialize_move(). If initialize_move() also does the job of trackable's move constructor, i.e. calls src.notify_callbacks(), it would not matter if trackable's and ObjectBase's default constructors are called instead of their move constructors.
Created attachment 314089 [details] [review] patch: ObjectBase, Object, Interface: Fix move constructors and move assignments Updated list of what I believe should be done, and what is done by the patch: - ObjectBase CC, CA deleted Protected MC, MA !!MC: Set gobject_ = nullptr, like the other ObjectBase constructors do. cpp_destruction_in_progress_ shall be initialized from src.cpp_destruction_in_progress_, not from src.custom_type_name_. !!MA: Add a self-assignment guard. Call g_object_steal_qdata() before the call to unreference(). A call to initialize_move() performs the rest of the job. - Object CC, CA deleted Public MC, MA !!MC: Should call sigc::trackable(std::move(src)) and src.notify_callbacks(). MA is ok. - Interface CC, CA deleted Public MC, MA !!MC: Should call sigc::trackable(std::move(src)). !!MA: Should not call ObjectBase::operator=(std::move(src)). (For the same reason that MC does not call ObjectBase::initialize_move(). ObjectBase::operator=() shall be called only once, and we assume that the most derived class, such as Gtk::Entry, also derives from Glib::Object.) - _CLASS_GOBJECT CC, CA deleted Public MC, MA are ok - _CLASS_INTERFACE CC, CA deleted Public MC, MA are ok
Review of attachment 314089 [details] [review]: I have pushed the patch https://git.gnome.org/browse/glibmm/commit/?id=2fbd9f23318abd07e446ea5251d1c2702ae27233