After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 756962 - Move operators in Glib::ObjectBase and its subclasses
Move operators in Glib::ObjectBase and its subclasses
Status: RESOLVED FIXED
Product: glibmm
Classification: Bindings
Component: object
2.46.x
Other All
: Normal normal
: ---
Assigned To: gtkmm-forge
gtkmm-forge
Depends on:
Blocks:
 
 
Reported: 2015-10-22 12:30 UTC by Kjell Ahlstedt
Modified: 2015-10-31 08:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch: ObjectBase, Object, Interface: Fix move constructors and move assignments (11.68 KB, patch)
2015-10-25 18:26 UTC, Kjell Ahlstedt
committed Details | Review

Description Kjell Ahlstedt 2015-10-22 12:30:35 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);
Comment 1 Kjell Ahlstedt 2015-10-23 08:52:42 UTC
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.
Comment 2 Kjell Ahlstedt 2015-10-25 18:26:01 UTC
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