Re: [PATCH] g_type_default_interface_ref(), etc.

[Owen Taylor <otaylor redhat com>]

On Fri, 2003-09-26 at 17:59, Tim Janik wrote:

> > +<!-- ##### 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.

Sure, switched.

> > +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.

Really? Why would you be looking at the docs for
g_type_default_interface_ref() when you wanted to find that information
out? I've added some text to the docs for class_init.

> > 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

I won't bother debating the removal here because as you say, it really
doesn't matter.

> - 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).

But cases like this are the one time that you are *absolutely* certain
that you know better than the compiler 

These are debug assertions - things that can never happen in functioning
cod. GCC may correctly predict it now, in the future, it may get even
smarter and stop correctly predicting it. We'll always get it right.

> > +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.

Fixed.

I've now committed the changes.

Regards,
						Owen