[glib/g-property: 9/26] gproperty: Clarify handling of complex types

[Emmanuele Bassi <ebassi src gnome org>]

commit 5f052c7212cdf2cd187026d38fab80308e8341c6
Author: Emmanuele Bassi <ebassi linux intel com>
Date:   Mon Jun 6 14:26:14 2011 +0100

    gproperty: Clarify handling of complex types
    
    The GProperty setters and getters deal with pointers; for complex types,
    a copy of the content will be made - but g_property_get() will return a
    pointer and leave the memory management decision to the implementation
    of the public-facing getter function.
    
    This change means that the GPropertyBoxedSet and GPropertyBoxedGet must
    return gpointer, not gconstpointer, to leave the chance of using
    refcounted boxed types up to the caller and implementation.
    
    The autoproperties test has been amended to cope with both cases, and
    verify the default implementation.

 gobject/gproperty.c            |   50 +++++++++++++----------
 gobject/gproperty.h            |    4 +-
 gobject/tests/autoproperties.c |   87 ++++++++++++++++++++++++++++++----------
 3 files changed, 96 insertions(+), 45 deletions(-)
---
diff --git a/gobject/gproperty.c b/gobject/gproperty.c
index 69d5500..69e5a5e 100644
--- a/gobject/gproperty.c
+++ b/gobject/gproperty.c
@@ -38,7 +38,8 @@
  * pointer to the private data structure as its last member, e.g.:
  *
  * |[
- *   typedef struct _TestObject TestObject;
+ *   typedef struct _TestObject         TestObject;
+ *   typedef struct _TestObjectPrivate  TestObjectPrivate;
  *
  *   struct _TestObject
  *   {
@@ -64,7 +65,8 @@
  *
  * |[
  * /&ast; the private data structure &ast;/
- * struct _TestObjectPrivate {
+ * struct _TestObjectPrivate
+ * {
  *   int x;
  *   int y;
  *   int width;
@@ -152,6 +154,13 @@
  *     return retval;
  *   }
  * ]|
+ *     <para>Note that calling g_property_set() for a property holding a
+ *     complex type (e.g. #GObject or #GBoxed) without a specific setter
+ *     function will result in the value being copied in the private data
+ *     structure's field. In contrast, calling g_property_get() will return
+ *     a pointer to the private data structure's field: it is up to the
+ *     getter function to decide whether to return a copy of the internal
+ *     data or the pointer itself.</para>
  *   </refsect3>
  *
  *   <refsect3>
@@ -2520,9 +2529,9 @@ g_boxed_property_validate (GProperty     *property,
 }
 
 static inline gboolean
-g_boxed_property_set_value (GProperty     *property,
-                            gpointer       gobject,
-                            gconstpointer  value)
+g_boxed_property_set_value (GProperty *property,
+                            gpointer   gobject,
+                            gpointer   value)
 {
   gboolean retval = FALSE;
 
@@ -2552,29 +2561,26 @@ g_boxed_property_set_value (GProperty     *property,
 
       retval = TRUE;
     }
-  if (property->field_offset >= 0)
+  else if (property->field_offset >= 0)
     {
       gpointer priv_p, field_p;
+      gpointer old_value;
 
       property_lock_internal (property, gobject);
 
       priv_p = get_private_pointer (gobject, property->priv_offset);
       field_p = G_STRUCT_MEMBER_P (priv_p, property->field_offset);
 
-      if ((* (gpointer *) field_p) == value)
-        {
-          property_unlock_internal (property, gobject);
-          return FALSE;
-        }
-
-      if ((* (gpointer *) field_p) != NULL)
-        g_boxed_free (((GParamSpec *) property)->value_type, (* (gpointer *) field_p));
+      old_value = (* (gpointer *) field_p);
 
       if (value != NULL)
         (* (gpointer *) field_p) = g_boxed_copy (((GParamSpec *) property)->value_type, value);
       else
         (* (gpointer *) field_p) = NULL;
 
+      if (old_value != NULL)
+        g_boxed_free (((GParamSpec *) property)->value_type, old_value);
+
       property_unlock_internal (property, gobject);
 
       g_object_notify_by_pspec (gobject, (GParamSpec *) property);
@@ -2589,7 +2595,7 @@ g_boxed_property_set_value (GProperty     *property,
   return retval;
 }
 
-static inline gconstpointer
+static inline gpointer
 g_boxed_property_get_value (GProperty *property,
                             gpointer   gobject)
 {
@@ -2605,14 +2611,16 @@ g_boxed_property_get_value (GProperty *property,
     {
       return ((GBoxedProperty *) property)->getter (gobject);
     }
-  if (property->field_offset >= 0)
+  else if (property->field_offset >= 0)
     {
       gpointer priv_p, field_p;
+      gpointer value;
 
       priv_p = get_private_pointer (gobject, property->priv_offset);
       field_p = G_STRUCT_MEMBER_P (priv_p, property->field_offset);
+      value = (* (gpointer *) field_p);
 
-      return (* (gpointer *) field_p);
+      return value;
     }
   else
     {
@@ -4423,7 +4431,7 @@ g_property_get_valist (GProperty *property,
       break;
 
     case G_TYPE_BOXED:
-      (* (gconstpointer *) ret_p) = g_boxed_property_get_value (property, gobject);
+      (* (gpointer *) ret_p) = g_boxed_property_get_value (property, gobject);
       break;
 
     case G_TYPE_OBJECT:
@@ -4850,10 +4858,10 @@ g_property_lcopy (GProperty *property,
 
               if ((flags & G_PROPERTY_COLLECT_COPY) != 0)
                 {
-                  if (boxed == NULL)
-                    (* (gpointer *) _cvalue->v_pointer) = NULL;
-                  else
+                  if (boxed != NULL)
                     (* (gpointer *) _cvalue->v_pointer) = g_boxed_copy (gtype, boxed);
+                  else
+                    (* (gpointer *) _cvalue->v_pointer) = NULL;
                 }
               else
                 (* (gpointer *) _cvalue->v_pointer) = (gpointer) boxed;
diff --git a/gobject/gproperty.h b/gobject/gproperty.h
index 41427f7..9b4e4ed 100644
--- a/gobject/gproperty.h
+++ b/gobject/gproperty.h
@@ -254,8 +254,8 @@ typedef void          (* GPropertyStringSet)  (gpointer       gobject,
 typedef const gchar * (* GPropertyStringGet)  (gpointer       gobject);
 
 typedef void          (* GPropertyBoxedSet)   (gpointer       gobject,
-                                               gconstpointer  value);
-typedef gconstpointer (* GPropertyBoxedGet)   (gpointer       gobject);
+                                               gpointer       value);
+typedef gpointer      (* GPropertyBoxedGet)   (gpointer       gobject);
 
 typedef void          (* GPropertyObjectSet)  (gpointer       gobject,
                                                gpointer       value);
diff --git a/gobject/tests/autoproperties.c b/gobject/tests/autoproperties.c
index 7eac768..2f2127b 100644
--- a/gobject/tests/autoproperties.c
+++ b/gobject/tests/autoproperties.c
@@ -122,6 +122,23 @@ test_flags_value_get_type (void)
   return g_define_type_id__volatile;
 }
 
+static TestBoxed *
+test_boxed_new (int x,
+                int y,
+                int width,
+                int height)
+{
+  TestBoxed *retval = g_new (TestBoxed, 1);
+
+  retval->x = x;
+  retval->y = y;
+  retval->width = width;
+  retval->height = height;
+  retval->ref_count = 1;
+
+  return retval;
+}
+
 static gpointer
 test_boxed_copy (gpointer data)
 {
@@ -129,6 +146,12 @@ test_boxed_copy (gpointer data)
     {
       TestBoxed *boxed = data;
 
+      if (g_test_verbose ())
+        g_print ("*** copy of boxed %p (ref count: %d) ***\n", boxed, boxed->ref_count);
+
+      if (boxed->ref_count < 0)
+        return test_boxed_new (boxed->x, boxed->y, boxed->width, boxed->height);
+
       boxed->ref_count += 1;
     }
 
@@ -142,6 +165,12 @@ test_boxed_free (gpointer data)
     {
       TestBoxed *boxed = data;
 
+      if (g_test_verbose ())
+        g_print ("*** free of boxed %p (ref count: %d) ***\n", boxed, boxed->ref_count);
+
+      if (boxed->ref_count < 0)
+        return;
+
       boxed->ref_count -= 1;
 
       if (boxed->ref_count == 0)
@@ -149,22 +178,7 @@ test_boxed_free (gpointer data)
     }
 }
 
-GType
-test_boxed_get_type (void)
-{
-  static volatile gsize g_define_type_id__volatile = 0;
-
-  if (g_once_init_enter (&g_define_type_id__volatile))
-    {
-      GType g_define_type_id =
-        g_boxed_type_register_static (g_intern_static_string ("TestBoxed"),
-                                      test_boxed_copy,
-                                      test_boxed_free);
-      g_once_init_leave (&g_define_type_id__volatile, g_define_type_id);
-    }
-
-  return g_define_type_id__volatile;
-}
+G_DEFINE_BOXED_TYPE (TestBoxed, test_boxed, test_boxed_copy, test_boxed_free)
 
 static GParamSpec *test_object_properties[LAST_PROP] = { NULL, };
 
@@ -180,7 +194,23 @@ G_DEFINE_PROPERTY_GET_SET (TestObject, test_object, float, width)
 G_DEFINE_PROPERTY_GET_SET (TestObject, test_object, double, x_align)
 G_DEFINE_PROPERTY_GET_SET (TestObject, test_object, TestEnumValue, enum_value)
 G_DEFINE_PROPERTY_GET_SET (TestObject, test_object, TestFlagsValue, flags_value)
-G_DEFINE_PROPERTY_GET_SET (TestObject, test_object, const TestBoxed *, boxed)
+
+G_DEFINE_PROPERTY_SET (TestObject, test_object, const TestBoxed *, boxed)
+
+void
+test_object_get_boxed (TestObject *self,
+                       TestBoxed  *value)
+{
+  TestBoxed *boxed;
+
+  g_property_get (G_PROPERTY (test_object_properties[PROP_BOXED]), self, &boxed);
+
+  /* make sure that g_property_get() didn't copy/ref the pointer */
+  g_assert (boxed == self->priv->boxed);
+  g_assert_cmpint (boxed->ref_count, ==, self->priv->boxed->ref_count);
+
+  *value = *boxed;
+}
 
 void
 test_object_set_str (TestObject  *self,
@@ -210,8 +240,21 @@ test_object_set_str (TestObject  *self,
 G_DEFINE_PROPERTY_GET (TestObject, test_object, const gchar *, str);
 
 static void
+test_object_finalize (GObject *gobject)
+{
+  TestObjectPrivate *priv = ((TestObject *) gobject)->priv;
+
+  test_boxed_free (priv->boxed);
+  g_free (priv->str);
+
+  G_OBJECT_CLASS (test_object_parent_class)->finalize (gobject);
+}
+
+static void
 test_object_class_init (TestObjectClass *klass)
 {
+  G_OBJECT_CLASS (klass)->finalize = test_object_finalize;
+
   g_type_class_add_private (klass, sizeof (TestObjectPrivate));
 
   test_object_properties[PROP_FOO] =
@@ -479,7 +522,8 @@ static void
 autoproperties_object_set (void)
 {
   TestObject *t = g_object_new (test_object_get_type (), NULL);
-  TestBoxed boxed = { 0, 0, 200, 200, 0 };
+  TestBoxed boxed = { 0, 0, 200, 200, -1 };
+  TestBoxed check;
 
   g_object_set (t,
                 "foo", 42,
@@ -491,10 +535,9 @@ autoproperties_object_set (void)
   g_assert_cmpint (test_object_get_foo (t), ==, 42);
   g_assert (test_object_get_bar (t));
   g_assert ((test_object_get_flags_value (t) & TEST_FLAGS_VALUE_BAZ) != 0);
-  g_assert (test_object_get_boxed (t) != NULL);
-  boxed = *test_object_get_boxed (t);
-  g_assert_cmpint (boxed.y, ==, 0);
-  g_assert_cmpint (boxed.width, ==, 200);
+  test_object_get_boxed (t, &check);
+  g_assert_cmpint (boxed.y, ==, check.y);
+  g_assert_cmpint (boxed.width, ==, check.width);
 
   g_object_unref (t);
 }