[glibmm] ObjectBase, Object, Interface: Fix move constructors and move assignments

[Kjell Ahlstedt <kjellahl src gnome org>]

commit 2fbd9f23318abd07e446ea5251d1c2702ae27233
Author: Kjell Ahlstedt <kjell ahlstedt bredband net>
Date:   Sat Oct 31 09:24:14 2015 +0100

    ObjectBase, Object, Interface: Fix move constructors and move assignments
    
    * glib/glibmm/interface.cc: Don't call ObjectBase's move assignment operator
    from Interface's move assignment operator.
    * glib/glibmm/object.cc: Perform of job of sigc::trackable's move constructor
    in Object's move constructor.
    * glib/glibmm/objectbase.cc: Move constructor: Set gobject_ = nullptr.
    Fix the assignment of cpp_destruction_in_progress_.
    Move assignment: Add self-assignment guard. Avoid the risk of accidentally
    deleting *this. Let a call to initialize_move() do most of the job.
    * tests/glibmm_interface_move/main.cc:
    * tests/glibmm_object_move/main.cc:
    * tests/glibmm_objectbase_move/main.cc: Really test move assignment.
    Test that the wrapped C object has been moved, and not copied.
    Bug #756962.

 glib/glibmm/interface.cc             |   20 ++++++++++++--------
 glib/glibmm/object.cc                |   10 ++++++----
 glib/glibmm/objectbase.cc            |   24 +++++++++++++++---------
 tests/glibmm_interface_move/main.cc  |   18 +++++++++++++-----
 tests/glibmm_object_move/main.cc     |   20 +++++++++++---------
 tests/glibmm_objectbase_move/main.cc |   22 +++++++++++++---------
 6 files changed, 70 insertions(+), 44 deletions(-)
---
diff --git a/glib/glibmm/interface.cc b/glib/glibmm/interface.cc
index 7af932c..9d2bf30 100644
--- a/glib/glibmm/interface.cc
+++ b/glib/glibmm/interface.cc
@@ -1,6 +1,3 @@
-// -*- c++ -*-
-/* $Id$ */
-
 /* Copyright (C) 2002 The gtkmm Development Team
  *
  * This library is free software; you can redistribute it and/or
@@ -118,18 +115,25 @@ Interface::Interface()
 {}
 
 Interface::Interface(Interface&& src) noexcept
-: ObjectBase(std::move(src))
+: sigc::trackable(std::move(src)), //not actually called because it's a virtual base
+  ObjectBase(std::move(src)) //not actually called because it's a virtual base
 {
-  //We don't call initialize_move() because we 
+  //We don't call initialize_move() because we
   //want the derived move constructor to only cause it
   //to be called once, so we just let it be called
-  //by the implementing class, such as Entry (implementing Editable).
+  //by the implementing class, such as Gtk::Entry (implementing Gtk::Editable
+  //and Gtk::CellEditable), via the call to Object::Object(Object&& src).
   //ObjectBase::initialize_move(src.gobject_, &src);
 }
 
-Interface& Interface::operator=(Interface&& src) noexcept
+Interface& Interface::operator=(Interface&& /* src */) noexcept
 {
-  ObjectBase::operator=(std::move(src));
+  //We don't call ObjectBase::operator=(ObjectBase&& src) because we
+  //want the derived move assignment operator to only cause it
+  //to be called once, so we just let it be called
+  //by the implementing class, such as Gtk::Entry (implementing Gtk::Editable
+  //and Gtk::CellEditable), via the call to Object::operator=(Object&& src).
+  //ObjectBase::operator=(std::move(src));
   return *this;
 }
 
diff --git a/glib/glibmm/object.cc b/glib/glibmm/object.cc
index 0373c5d..2f20189 100644
--- a/glib/glibmm/object.cc
+++ b/glib/glibmm/object.cc
@@ -1,6 +1,3 @@
-// -*- c++ -*-
-/* $Id$ */
-
 /* Copyright 1998-2002 The gtkmm Development Team
  *
  * This library is free software; you can redistribute it and/or
@@ -286,8 +283,13 @@ Object::Object(GObject* castitem)
 }
 
 Object::Object(Object&& src) noexcept
-: ObjectBase(std::move(src)) //not actually called because it's a virtual base
+: sigc::trackable(std::move(src)), //not actually called because it's a virtual base
+  ObjectBase(std::move(src)) //not actually called because it's a virtual base
 {
+  // Perhaps trackable's move constructor has not been called. Do its job here.
+  // (No harm is done if notify_callbacks() is called twice. The second call
+  // won't do anything.)
+  src.notify_callbacks();
   ObjectBase::initialize_move(src.gobject_, &src);
 }
 
diff --git a/glib/glibmm/objectbase.cc b/glib/glibmm/objectbase.cc
index 080949d..874958f 100644
--- a/glib/glibmm/objectbase.cc
+++ b/glib/glibmm/objectbase.cc
@@ -113,22 +113,28 @@ void ObjectBase::initialize_move(GObject* castitem, Glib::ObjectBase* previous_w
 }
 
 ObjectBase::ObjectBase(ObjectBase&& src) noexcept
-: sigc::trackable(std::move(src)),
-  gobject_(std::move(src.gobject_)),
-  custom_type_name_(std::move(src.custom_type_name_)),
-  cpp_destruction_in_progress_(std::move(src.custom_type_name_))
+: sigc::trackable(std::move(src)), //not actually called because it's a virtual base
+  gobject_(nullptr),
+  custom_type_name_(src.custom_type_name_),
+  cpp_destruction_in_progress_(src.cpp_destruction_in_progress_)
 {}
 
 ObjectBase& ObjectBase::operator=(ObjectBase&& src) noexcept
 {
+  if (this == &src)
+    return *this;
+
   sigc::trackable::operator=(std::move(src));
 
-  if(gobject_)
+  if (gobject_)
+  {
+    // Remove the wrapper, without invoking destroy_notify_callback_():
+    g_object_steal_qdata(gobject_, Glib::quark_);
+    // Remove a reference, without deleting *this.
     unreference();
-
-  gobject_ = std::move(src.gobject_);
-  custom_type_name_ = std::move(src.custom_type_name_);
-  cpp_destruction_in_progress_ = std::move(src.custom_type_name_);
+    gobject_ = nullptr;
+  }
+  initialize_move(src.gobject_, &src);
 
   return *this;
 }
diff --git a/tests/glibmm_interface_move/main.cc b/tests/glibmm_interface_move/main.cc
index 29662e9..28b8e8f 100644
--- a/tests/glibmm_interface_move/main.cc
+++ b/tests/glibmm_interface_move/main.cc
@@ -106,7 +106,7 @@ protected:
 
 public:
   //A real application would never make the constructor public.
-  //It would instead have a protectd constructor and a public create() method.
+  //It would instead have a protected constructor and a public create() method.
   TestInterface(GObject* gobject, int i)
   : Glib::Interface(gobject),
     i_(i)
@@ -220,7 +220,10 @@ private:
 
 DerivedObject::CppClassType DerivedObject::derived_object_class_; // initialize static member
 
-/* TODO: Shouldn't this work too?
+/* Shouldn't this work too?
+ * No, because Glib::Interface::Interface(Interface&& src) does not call
+ * Glib::ObjectBase::initialize_move(), and Glib::Interface::operator=(Interface&& src)
+ * does not call Glib::ObjectBase::operator=(std::move(src)).
 static
 void test_interface_move_constructor()
 {
@@ -258,11 +261,13 @@ void test_object_with_interface_move_constructor()
 {
   DerivedObject derived(5);
   g_assert_cmpint(derived.i_, ==, 5);
-  GObject *gobject = derived.gobj();
+  GObject* gobject = derived.gobj();
   g_assert(derived.gobj() == gobject);
+
   DerivedObject derived2(std::move(derived));
   g_assert_cmpint(derived2.i_, ==, 5);
   g_assert(derived2.gobj() == gobject);
+  g_assert(derived.gobj() == nullptr);
 }
 
 static
@@ -270,11 +275,14 @@ void test_object_with_interface_move_assignment_operator()
 {
   DerivedObject derived(5);
   g_assert_cmpint(derived.i_, ==, 5);
-  GObject *gobject = derived.gobj();
+  GObject* gobject = derived.gobj();
   g_assert(derived.gobj() == gobject);
-  DerivedObject derived2 = std::move(derived);
+
+  DerivedObject derived2(6);
+  derived2 = std::move(derived);
   g_assert_cmpint(derived2.i_, ==, 5);
   g_assert(derived2.gobj() == gobject);
+  g_assert(derived.gobj() == nullptr);
 }
 
 
diff --git a/tests/glibmm_object_move/main.cc b/tests/glibmm_object_move/main.cc
index a62bcf9..7868b17 100644
--- a/tests/glibmm_object_move/main.cc
+++ b/tests/glibmm_object_move/main.cc
@@ -2,7 +2,7 @@
 #include <iostream>
 #include <stdlib.h>
 
-//A basic derived GObject, just to test Glib::ObjectBase.
+//A basic derived GObject, just to test Glib::Object.
 typedef struct {
     GObject parent;
 } TestDerived;
@@ -27,7 +27,7 @@ class DerivedObject : public Glib::Object
 {
 public:
   //A real application would never make the constructor public.
-  //It would instead have a protectd constructor and a public create() method.
+  //It would instead have a protected constructor and a public create() method.
   DerivedObject(GObject* gobject, int i)
   : Glib::Object(gobject),
     i_(i)
@@ -56,32 +56,34 @@ public:
 static
 void test_object_move_constructor()
 {
-  GObject *gobject = G_OBJECT(g_object_new(TEST_TYPE_DERIVED, nullptr));
-  g_object_ref(gobject);
-
+  GObject* gobject = G_OBJECT(g_object_new(TEST_TYPE_DERIVED, nullptr));
   DerivedObject derived(gobject, 5);
   std::cout << "debug: gobj(): " << derived.gobj() << std::endl;
   g_assert(derived.gobj() == gobject);
+
   DerivedObject derived2(std::move(derived));
   g_assert_cmpint(derived2.i_, ==, 5);
   std::cout << "debug: gobj(): " << derived2.gobj() << std::endl;
   g_assert(derived2.gobj() == gobject);
+  g_assert(derived.gobj() == nullptr);
 }
 
 
 static
 void test_object_move_assignment_operator()
 {
-  GObject *gobject = G_OBJECT(g_object_new(TEST_TYPE_DERIVED, nullptr));
-  g_object_ref(gobject);
-
+  GObject* gobject = G_OBJECT(g_object_new(TEST_TYPE_DERIVED, nullptr));
   DerivedObject derived(gobject, 5);
   //std::cout << "debug: gobj(): " << derived.gobj() << std::endl;
   g_assert(derived.gobj() == gobject);
-  DerivedObject derived2 = std::move(derived);
+
+  GObject* gobject2 = G_OBJECT(g_object_new(TEST_TYPE_DERIVED, nullptr));
+  DerivedObject derived2(gobject2, 6);
+  derived2 = std::move(derived);
   g_assert_cmpint(derived2.i_, ==, 5);
   //std::cout << "debug: gobj(): " << derived2.gobj() << std::endl;
   g_assert(derived2.gobj() == gobject);
+  g_assert(derived.gobj() == nullptr);
 }
 
 
diff --git a/tests/glibmm_objectbase_move/main.cc b/tests/glibmm_objectbase_move/main.cc
index ed71fe1..e70ff3b 100644
--- a/tests/glibmm_objectbase_move/main.cc
+++ b/tests/glibmm_objectbase_move/main.cc
@@ -28,7 +28,7 @@ class DerivedObjectBase : public Glib::ObjectBase
 {
 public:
   //A real application would never make the constructor public.
-  //It would instead have a protectd constructor and a public create() method.
+  //It would instead have a protected constructor and a public create() method.
   DerivedObjectBase(GObject* gobject, int i)
   : Glib::ObjectBase(nullptr),
     i_(i)
@@ -42,7 +42,9 @@ public:
   DerivedObjectBase(DerivedObjectBase&& src) noexcept
   : Glib::ObjectBase(std::move(src)),
     i_(std::move(src.i_))
-  {}
+  {
+    ObjectBase::initialize_move(src.gobject_, &src);
+  }
 
   DerivedObjectBase& operator=(DerivedObjectBase&& src) noexcept
   {
@@ -58,31 +60,33 @@ public:
 static
 void test_objectbase_move_constructor()
 {
-  GObject *gobject = G_OBJECT(g_object_new(TEST_TYPE_DERIVED, nullptr));
-  g_object_ref(gobject);
-
+  GObject* gobject = G_OBJECT(g_object_new(TEST_TYPE_DERIVED, nullptr));
   DerivedObjectBase derived(gobject, 5);
   //std::cout << "debug: gobj(): " << derived.gobj() << std::endl;
   g_assert(derived.gobj() == gobject);
+
   DerivedObjectBase derived2(std::move(derived));
   g_assert_cmpint(derived2.i_, ==, 5);
   //std::cout << "debug: gobj(): " << derived2.gobj() << std::endl;
   g_assert(derived2.gobj() == gobject);
+  g_assert(derived.gobj() == nullptr);
 }
 
 static
 void test_objectbase_move_assignment_operator()
 {
-  GObject *gobject = G_OBJECT(g_object_new(TEST_TYPE_DERIVED, nullptr));
-  g_object_ref(gobject);
-
+  GObject* gobject = G_OBJECT(g_object_new(TEST_TYPE_DERIVED, nullptr));
   DerivedObjectBase derived(gobject, 5);
   //std::cout << "debug: gobj(): " << derived.gobj() << std::endl;
   g_assert(derived.gobj() == gobject);
-  DerivedObjectBase derived2 = std::move(derived);
+
+  GObject* gobject2 = G_OBJECT(g_object_new(TEST_TYPE_DERIVED, nullptr));
+  DerivedObjectBase derived2(gobject2, 6);
+  derived2 = std::move(derived);
   g_assert_cmpint(derived2.i_, ==, 5);
   //std::cout << "debug: gobj(): " << derived2.gobj() << std::endl;
   g_assert(derived2.gobj() == gobject);
+  g_assert(derived.gobj() == nullptr);
 }
 
 int main(int, char**)