Re: Another version of instance-private-data [really!]

[Tim Janik <timj gtk org>]

On Mon, 3 Mar 2003, Owen Taylor wrote:

> The attached patch fixes and tests getting the
> private data for an ancestor type of the instance
> type.

> Index: gobject/gtype.c
> ===================================================================
> RCS file: /cvs/gnome/glib/gobject/gtype.c,v
> retrieving revision 1.59
> diff -u -p -r1.59 gtype.c
> --- gobject/gtype.c	7 Feb 2003 22:04:24 -0000	1.59
> +++ gobject/gtype.c	3 Mar 2003 18:00:05 -0000
> @@ -107,6 +107,18 @@ static GStaticRWLock            type_rw_
>  						       sizeof (gpointer)), \
>                                                    sizeof (glong)))
>
> +/* The 2*sizeof(size_t) alignment here is borrowed from
> + * GNU libc, so it should be good most everywhere.
> + * It is more conservative than is needed on some 64-bit
> + * platforms, but ia64 does require a 16-byte alignment.
> + * The SIMD extensions for x86 and ppc32 would want a
> + * larger alignment than this, but we don't need to
> + * do better than malloc.
> + */
> +#define STRUCT_ALIGNMENT (2 * sizeof (gsize))
> +#define ALIGN_STRUCT(offset) \
> +      ((offset + (STRUCT_ALIGNMENT - 1)) & -STRUCT_ALIGNMENT)
> +

there's alignment logic in the definition of
SIZEOF_FUNDAMENTAL_INFO already implemented, it should be
seperated and reused (and is well tested on a number of
systems already, including ia64).

>  /* --- typedefs --- */
>  typedef struct _TypeNode        TypeNode;
> @@ -229,6 +241,7 @@ struct _InstanceData
>    gconstpointer      class_data;
>    gpointer           class;
>    guint16            instance_size;
> +  guint16            private_size;
>    guint16            n_preallocs;
>    GInstanceInitFunc  instance_init;
>    GMemChunk        *mem_chunk;
> @@ -909,6 +922,13 @@ type_data_make_W (TypeNode
>        data->instance.class_data = info->class_data;
>        data->instance.class = NULL;
>        data->instance.instance_size = info->instance_size;
> +      if (NODE_PARENT_TYPE (node))
> +	{
> +	  TypeNode *pnode = lookup_type_node_I (NODE_PARENT_TYPE (node));
> +	  data->instance.private_size = pnode->data->instance.private_size;
> +	}
> +      else
> +	data->instance.private_size = 0;
>  #ifdef	DISABLE_MEM_POOLS
>        data->instance.n_preallocs = 0;
>  #else	/* !DISABLE_MEM_POOLS */
> @@ -1347,6 +1367,7 @@ g_type_create_instance (GType type)
>    GTypeInstance *instance;
>    GTypeClass *class;
>    guint i;
> +  gsize total_instance_size;
>
>    node = lookup_type_node_I (type);
>    if (!node || !node->is_instantiatable)
> @@ -1364,6 +1385,10 @@ g_type_create_instance (GType type)
>      }
>
>    class = g_type_class_ref (type);
> +
> +  total_instance_size = node->data->instance.instance_size;
> +  if (node->data->instance.private_size != 0)
> +    total_instance_size = ALIGN_STRUCT (total_instance_size) + node->data->instance.private_size;
>
>    if (node->data->instance.n_preallocs)
>      {
> @@ -1371,14 +1396,14 @@ g_type_create_instance (GType type)
>        if (!node->data->instance.mem_chunk)
>  	node->data->instance.mem_chunk = g_mem_chunk_new (NODE_NAME (node),
>  							  node->data->instance.instance_size,
> -							  (node->data->instance.instance_size *
> +							  (total_instance_size *
>  							   node->data->instance.n_preallocs),
>  							  G_ALLOC_AND_FREE);
>        instance = g_chunk_new0 (GTypeInstance, node->data->instance.mem_chunk);
>        G_WRITE_UNLOCK (&type_rw_lock);
>      }
>    else
> -    instance = g_malloc0 (node->data->instance.instance_size);	/* fine without read lock */
> +    instance = g_malloc0 (total_instance_size);	/* fine without read lock */
>    for (i = node->n_supers; i > 0; i--)
>      {
>        TypeNode *pnode;
> @@ -1424,7 +1449,7 @@ g_type_free_instance (GTypeInstance *ins
>
>    instance->g_class = NULL;
>  #ifdef G_ENABLE_DEBUG
> -  memset (instance, 0xaa, node->data->instance.instance_size);	/* debugging hack */
> +  memset (instance, 0xaa, total_instance_size);	/* debugging hack */

i fail to see how this could compile, since you added total_instance_size
to g_type_create_instance() only.

>  #endif
>    if (node->data->instance.n_preallocs)
>      {
> @@ -1527,7 +1552,7 @@ type_class_init_Wm (TypeNode   *node,
>    g_assert (node->is_classed && node->data &&
>  	    node->data->class.class_size &&
>  	    !node->data->class.class);
> -
> +
>    class = g_malloc0 (node->data->class.class_size);
>    node->data->class.class = class;
>
> @@ -3024,4 +3049,80 @@ void
>  g_type_init (void)
>  {
>    g_type_init_with_debug_flags (0);
> +}
> +
> +void
> +g_type_class_add_private (gpointer g_class,
> +			  gsize    private_size)
> +{
> +  GType instance_type = ((GTypeClass *)g_class)->g_type;
> +  TypeNode *node = lookup_type_node_I (instance_type);
> +  gsize offset;
> +
> +  if (!node || !node->is_instantiatable || !node->data || node->data->class.class != (gpointer) g_class)

there's no need to cast g_class into a (gpointer) which it is already.

> +    {
> +      g_warning ("cannot add private field to invalid (non-instantiatable) type '%s'",
> +		 type_descriptive_name_I (instance_type));
> +      return;
> +    }
> +
> +  if (NODE_PARENT_TYPE (node))
> +    {
> +      TypeNode *pnode = lookup_type_node_I (NODE_PARENT_TYPE (node));
> +      if (node->data->instance.private_size != pnode->data->instance.private_size)
> +	{
> +	  g_warning ("g_type_add_private() called multiple times for the same type");
> +	  return;
> +	}
> +    }
> +
> +  G_WRITE_LOCK (&type_rw_lock);
> +
> +  offset = ALIGN_STRUCT (node->data->instance.private_size);
> +  node->data->instance.private_size = offset + private_size;
> +
> +  G_WRITE_UNLOCK (&type_rw_lock);
> +}
> +
> +gpointer
> +g_type_instance_get_private (GTypeInstance *instance,
> +			     GType          private_type)
> +{
> +  TypeNode *instance_node;
> +  TypeNode *private_node;
> +  TypeNode *parent_node;
> +  gsize offset;
> +
> +  g_return_val_if_fail (instance != NULL && instance->g_class != NULL, NULL);
> +
> +  instance_node = lookup_type_node_I (instance->g_class->g_type);
> +  if (G_UNLIKELY (!instance_node || !instance_node->is_instantiatable))
> +    {
> +      g_warning ("instance of invalid non-instantiatable type `%s'",
> +		 type_descriptive_name_I (instance->g_class->g_type));
> +      return NULL;
> +    }
> +
> +  private_node = lookup_type_node_I (private_type);
> +  if (G_UNLIKELY (!private_node || !private_node->is_instantiatable))
> +    {
> +      g_warning ("cannot retrieve private instant for non-instantiatable type '%s'",
> +		 type_descriptive_name_I (private_type));
> +      return NULL;
> +    }

we better check whether private_node is a parent node of instance,
rather than just checking whether it's instantiatable (which is
a pretty arbitrary check for an assumed parent type), that is:

  if (G_UNLIKELY (!private_node ||
                  private_node->n_supers > instance_node->n_supers ||
                  instance_node->supers[instance_node->n_supers -
                                        private_node->n_supers] !=
                  NODE_TYPE (private_node)))

> +
> +  /* Note that we don't need a read lock, since instance existing
> +   * means that the instance class and all parent classes
> +   * exist, so the node->data, node->data->instance.instance_size,
> +   * and node->data->instance.private_size are not going to be changed.
> +   * for any of the relevant types.
> +   */
> +
> +  parent_node = lookup_type_node_I (NODE_PARENT_TYPE (private_node));
> +
> +  g_assert (parent_node->data && parent_node->data->common.ref_count);

bad assertion, parent_node might be NULL if you request private data
from a fundamental type instance.

> +
> +  offset = ALIGN_STRUCT (instance_node->data->instance.instance_size) + parent_node->data->instance.private_size;

so better do:

  offset = ALIGN_STRUCT (instance_node->data->instance.instance_size);
  if (parent_node)
    offset += ALIGN_STRUCT (parent_node->data->instance.private_size);
(using ALIGN_STRUCT() in this place to correspond to the ALIGN_STRUCT()
you used in g_type_class_add_private())

> +
> +  return (gpointer)((gchar *)instance + ALIGN_STRUCT (offset));

glib has a macro for this, so the code should read:

  return G_STRUCT_MEMBER_P (instance, offset);
(ALIGN_STRUCT() not needed with the above if (parent_node) branch)

>  }


---
ciaoTJ