Re: gdk_threads_enter/leave & OO.o ...

[Owen Taylor <otaylor redhat com>]

On Tue, 2003-12-02 at 17:59, Michael Meeks wrote:

> +2003-12-02  Michael Meeks  <michael ximian com>
> +
> +	* gdk/gdk.c (gdk_threads_set_guard_hooks): impl.
> +
> +Tue Sep 16 15:46:31  Martin Kretzschmar  <m_kretzschmar gmx net>
> +
> +	* gdk/gdk.h: new gdk_threads_lock, gdk_threads_unlock, point to
> +	implementation of GDK_THREADS_ENTER / GDK_THREADS_LEAVE.
> +	(GDK_THREADS_ENTER, GDK_THREADS_LEAVE): use gdk_threads_[un]lock
> +	function pointers. Deprecate the global gdk_threads_mutex variable.
> +	
> +	* gdk/gdk.c (gdk_threads_impl_lock, gdk_threads_impl_unlock): new,
> +	extracted from GTK_THREADS_ENTER/LEAVE macros.
> +	(gdk_threads_init): init gtk_threads_[un]lock.
> +
> +	* gdk/gdkglobals.c: add definitions of gdk_threads_[un]lock.
> +
> +	* gdk/gdktypes.h: new GdkGuardFunction typedef for
> +	gdk_threads_[un]lock.
> +
>  2003-12-01  Federico Mena Quintero  <federico ximian com>
>  
>  	Decouple impl->current_folder from the selection in the folder

Could you clean this up to have one ChangeLog entry "Based on a patch
by Martin Kretzschmar" or whatever?

> Index: gdk/gdk.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gdk/gdk.c,v
> retrieving revision 1.152
> diff -u -p -u -r1.152 gdk.c
> --- gdk/gdk.c	3 Nov 2003 20:08:30 -0000	1.152
> +++ gdk/gdk.c	2 Dec 2003 14:53:07 -0000
> @@ -495,6 +495,22 @@ gdk_threads_leave ()
>    GDK_THREADS_LEAVE ();
>  }
>  
> +GDKVAR GMutex *gdk_threads_mutex;

Instead of duplicating this, what we do is:

#if !defined(GTK_DISABLE_DEPRECATED) || defined(GDK_COMPILATION)

in gdk.h.

> +static void
> +gdk_threads_impl_lock ()

We use (void) even in definitions.

[...]

> +/**
> + * gdk_threads_set_guard_hooks:
> + * @fn_lock:   function called to guard gtk+
> + * @fn_unlock: function called to release the guard
> + * 
> + * This method allows a one-shot override of the global
> + * gtk lock used to ensure that only one thread can be
> + * using gtk/X. This allows an application implementing
> + * it's own threading model to synchronize it's own internal

'its' :-) 

> + * locking scheme with that of gtk+.

Allows the application to replace the standard method that
GDK uses to protect its data structures. Normally, GDK
creates a single #GMutex that is locked by 
gdk_threads_enter(), and released by gdk_threads_leave();
using this function an application provides, instead,
a function @lock that is called by gdk_threads_enter()
and a function @unlock that is called by gdk_threads_leave().

[ Maybe rename the parameters to @enter_function, @leave_function ? ]

The functions must provide at least same locking functionality
as the default implementation, but can also do extra 
application specific processing.

> + * This is particularly useful for allowing gtk+ code that
> + * enters a new mainloop, eg. gtk_dialog_run to drop the
> + * application's own lock, such that it can perform other
> + * graphical methods on other threads while waiting for a
> + * response.

As an example, consider an application that has its own 
recursive lock that when held, holds the GTK+ lock as well.
When GTK+ unlocks the GTK+ lock when entering a recursive
main loop, the application must temporarily release all
its lock as well.

> + * This method should not be used in normal applications,

I don't think this comment makes sense; how do you define
a normal application? You could say maybe: "Most threaded
GTK+ apps won't need to do this"

> + * It must be called after gdk_threads_init.

I think it would be better to make it "before" - that is going to
require some internal variables to hold the pointers temporarily,
but makes more sense for the user, and is easier to check.

And we don't need to add the qualification

 "It must be called after gdk_threads_init(), but before any
  calls to gdk_threads_enter(), or gdk_main(), or gdk_dialog_run()
  or similar functions"

And, I'd mention the one-shot nature here rather than in passing
above

 "This function must be called before gdk_threads_init() and cannot
  be called multiple times."

> + **/
> +void
> +gdk_threads_set_guard_hooks (GdkGuardFunction fn_lock,
> +			     GdkGuardFunction fn_unlock)

See naming comments below.

> +{
> +	gdk_threads_lock = fn_lock;
> +	gdk_threads_unlock = fn_unlock;

If we want this to be one-shot, we should enforce that. Unenforced
documented constraints just get ignored.

[...]

> +void     gdk_threads_set_guard_hooks      (GdkGuardFunction fn_lock,
> +					   GdkGuardFunction fn_unlock);

I don't really like the name "Guard" here. 

 gdk_threads_set_lock_functions (GCallback lock_function,
                                 GCallback unlock_function);

Maybe? (Should this take a gpointer user_data? Then we'd probably
want a new typedef. But since it's just a single global function,
the caller can use a static easily enough.)

>  #ifdef	G_THREADS_ENABLED
>  #  define GDK_THREADS_ENTER()	G_STMT_START {	\
> -      if (gdk_threads_mutex)                 	\
> -        g_mutex_lock (gdk_threads_mutex);   	\
> +      if (gdk_threads_lock)                 	\
> +        gdk_threads_lock ();			\

I have a small preference for writing this

 (*gdk_threads_lock) ();

which means absolutely the same thing, but has the advantage of making
it clear what is going on.


> -GMutex           *gdk_threads_mutex = NULL;          /* Global GDK lock
> */
> -
> +GMutex              *gdk_threads_mutex = NULL;          /* Global GDK
> lock; deprecated */
> +GdkGuardFunction     gdk_threads_lock = NULL;
> +GdkGuardFunction     gdk_threads_unlock = NULL;

It still is used internally. Maybe 'public use is deprecated'. Or just
omit mention of deprecation.

> Index: gdk/gdktypes.h
> ===================================================================
[..]
>  
> +typedef void (*GdkGuardFunction) (void);

This should go in gdk.h - the gdktypes.h location for all types is
obsolete. But we might not need it at all. See above.

Regards,
						Owen