Re: [PATCH] g_type_default_interface_ref(), etc.

[Tim Janik <timj gtk org>]

On Thu, 25 Sep 2003, Owen Taylor wrote:

> On Thu, 2003-09-25 at 17:25, Owen Taylor wrote:
> > Here's a patch for:
> >
> >  g_type_default_interface_ref()
> >  g_type_default_interface_peek()
> >  g_type_default_interface_unref()
> >
> > as described in your last mail, with docs and a test case
> > for both static and dynamic interfaces.
>
> And the patch...


the patch has some general glib doc section junk and lacks a
changelog entry for the new gtype functions.

> Index: docs/reference/gobject/tmpl/gtype.sgml
> ===================================================================
> RCS file: /cvs/gnome/glib/docs/reference/gobject/tmpl/gtype.sgml,v
> retrieving revision 1.24
> diff -u -p -u -r1.24 gtype.sgml
> --- docs/reference/gobject/tmpl/gtype.sgml	2 Sep 2003 17:57:21 -0000	1.24
> +++ docs/reference/gobject/tmpl/gtype.sgml	25 Sep 2003 21:22:30 -0000
> @@ -774,6 +774,50 @@ then possibly overriding some methods.
>     doesn't conform to the interface.
>
>
> +<!-- ##### FUNCTION g_type_default_interface_ref ##### -->
> +<para>
> +Increments the reference count for the interface type @g_type,
> +and returns the default interface vtable for the type.
> +</para>
> +<para>
> +If the type is not currently in use, then the default vtable
> +for the type will be created and initalized by calling
> +the default vtable init and base interface init functions for

the functions should be mentioned in the order they are called in,
i.e. base-init then init.

> +the type (the <structfield>class_init</structfield>,
> +and <structfield>base_init</structfield> members of #GTypeInfo).
> +Calling g_type_default_interface_ref() is useful when you
> +want to make sure that signals and properties for an interface
> +have been installed.

this would be the place then to mention that class_init in
typeinfo of interfaces is the proper place for signal/property
initialization.

> +</para>
> +
> + g_type: an interface type
> + Returns: the default vtable for the interface; call
> + g_type_default_interface_unref() when you are done using
> + the interface.
> +
> +<!-- ##### FUNCTION g_type_default_interface_peek ##### -->
> +<para>
> +If the interface type @g_type is currently in use, returns
> +its default interface vtable.
> +</para>
> +
> + g_type: an interface type
> + Returns: the default vtable for the interface; or %NULL
> + if the type is not currently in use.
> +
> +<!-- ##### FUNCTION g_type_default_interface_unref ##### -->
> +<para>
> +Decrements the reference count for the type corresponding to the
> +interface default vtable @g_iface. If the type is dynamic, then
> +when no one is using the interface and all references have
> +been released, the finalize function for the interface's default
> +vtable (the <structfield>class_finalize</structfield> member of
> +#GTypeInfo) will be called.
> +</para>
> +
> + g_iface: the default vtable structure for a interface, as
> +  returned by g_type_default_interface_ref()
> +
>  <!-- ##### FUNCTION g_type_children ##### -->
>  <para>
>  Return a newly allocated and 0-terminated array of type IDs, listing the
> Index: gobject/gtype.c
> ===================================================================
> RCS file: /cvs/gnome/glib/gobject/gtype.c,v
> retrieving revision 1.66
> diff -u -p -u -r1.66 gtype.c
> --- gobject/gtype.c	2 Sep 2003 17:57:22 -0000	1.66
> +++ gobject/gtype.c	25 Sep 2003 21:22:31 -0000
> @@ -2486,6 +2486,69 @@ g_type_interface_peek_parent (gpointer g
>    return vtable;
>  }
>
> +gpointer
> +g_type_default_interface_ref (GType g_type)
> +{
> +  TypeNode *node;
> +
> +  G_WRITE_LOCK (&type_rw_lock);
> +
> +  node = lookup_type_node_I (g_type);
> +  if (G_UNLIKELY (!node || !NODE_IS_IFACE (node) ||
> +		  (node->data && node->data->common.ref_count < 1)))
> +    {

don't use G_UNLIKELY() here:
- the code here isn't time critical enough that a couple extra asm instructions
  make a difference
- in relation to what the rest of this function does, likelyness
  optimization here becomes rightous pointless
- you're hinting likelyness for en error checking scenarion that gcc will hint
  correctly very easily (you're branching around a return; statement)

in general, the default behaviour should be to not hint branch likelyness
as programmers are notoriously bad at guessing what their code does and
since most cases are better handled by gcc's prediction logic anyways (which
is quite sophisticated).
if you still think likelyness hinting would be usefull:
- make sure the code being hinted is reasonably time critical by running
  actual profiling tests
- make sure you've read up about gcc's latest version prediction facilities
  to avoid (possibly false) hints where gcc guesses better already or will soon


> +      G_WRITE_UNLOCK (&type_rw_lock);
> +      g_warning ("cannot retrieve default vtable for invalid or non-interface type '%s'",
> +		 type_descriptive_name_I (g_type));
> +      return NULL;
> +    }
> +
> +  type_data_ref_Wm (node);
> +
> +  type_iface_ensure_dflt_vtable_Wm (node);
> +
> +  G_WRITE_UNLOCK (&type_rw_lock);
> +
> +  return node->data->iface.dflt_vtable;
> +}
> +
> +gpointer
> +g_type_default_interface_peek (GType g_type)
> +{
> +  TypeNode *node;
> +  gpointer class;

please use GTypeInterface *vtable; instead of "class" here, i made a point
in avoiding calling vtables classes throughout the code since a class is a
different type of beast for the scope of gtype.c.

> +
> +  node = lookup_type_node_I (g_type);
> +  G_READ_LOCK (&type_rw_lock);
> +  if (node && NODE_IS_IFACE (node) && node->data && node->data->iface.dflt_vtable)
> +    class = node->data->iface.dflt_vtable;
> +  else
> +    class = NULL;
> +  G_READ_UNLOCK (&type_rw_lock);
> +
> +  return class;
> +}
> +
> +void
> +g_type_default_interface_unref (gpointer g_iface)
> +{
> +  TypeNode *node;
> +  GTypeInterface *vtable = g_iface;
> +
> +  g_return_if_fail (g_iface != NULL);
> +
> +  node = lookup_type_node_I (vtable->g_type);
> +  G_WRITE_LOCK (&type_rw_lock);
> +  if (G_LIKELY (node && NODE_IS_IFACE (node) &&
> +		node->data->iface.dflt_vtable == g_iface &&
> +		node->data->common.ref_count > 0))
> +    type_data_unref_Wm (node, FALSE);

please remove the G_LIKELY() hinting here also.

> +  else
> +    g_warning ("cannot unreference invalid interface default vtable for '%s'",
> +	       type_descriptive_name_I (vtable->g_type));
> +  G_WRITE_UNLOCK (&type_rw_lock);
> +}
> +
>  G_CONST_RETURN gchar*
>  g_type_name (GType type)
>  {
> Index: gobject/gtype.h
> ===================================================================
> RCS file: /cvs/gnome/glib/gobject/gtype.h,v
> retrieving revision 1.49
> diff -u -p -u -r1.49 gtype.h
> --- gobject/gtype.h	2 Sep 2003 17:57:22 -0000	1.49
> +++ gobject/gtype.h	25 Sep 2003 21:22:31 -0000
> @@ -183,6 +183,10 @@ gpointer              g_type_interface_p
>  						      GType            iface_type);
>  gpointer              g_type_interface_peek_parent   (gpointer         g_iface);
>
> +gpointer              g_type_default_interface_ref   (GType            g_type);
> +gpointer              g_type_default_interface_peek  (GType            g_type);
> +void                  g_type_default_interface_unref (gpointer         g_iface);
> +
>  /* g_free() the returned arrays */
>  GType*                g_type_children                (GType            type,
>  						      guint           *n_children);
[additional tests not reviewed]


the patch looks generally good, please comitt once you sorted
out the above minor issues.

---
ciaoTJ