I use this style in my code that uses cleanup functions; basically
anonymous inner scopes:

static void
my_function()
{
  static GMutex mutex;

  /* Stuff that don't need the lock */

  { g_auto(GMutexLocker) locker = G_MUTEX_LOCKER_INIT (&mutex);
    /* Note new anonymous scope */

    if (cond) {
      /* No need to unlock ! */
      return;
    }
  }

  /* Now the mutex is unlocked here too */
}

Created attachment 296154 [details] [review]
Add GMutexLocker

Wanted to play with the idea, basically just copied QMutexLocker.

@Colin: you mean you would use my proposed GMutexLocker or your code snippet is wrong and you have another way of doing it?

Example how to use my proposed API:

{
  static GMutex mutex;
  g_auto(GMutexLocker) locker = g_mutex_locker_init (&mutex);

  /* Here you have the lock */

  ...
  if (cond)
    /* No need to unlock ! */
    return;
  ...

  g_mutex_locker_unlock (&locker);

  /* Here you don't have the lock */
}

I meant to demonstrate with my code example that one can define a variant of this cleanup function that does not need an additional "locked" boolean.

Ok, I see, can indeed be stripped down to this if we don't want to let unlock/relock:

typedef GMutex* GMutexLocker;

static inline GMutexLocker
g_mutex_locker_init (GMutex *mutex)
{
  g_mutex_lock (mutex);
  return mutex;
}

static inline void
g_mutex_locker_clear (GMutexLocker *locker)
{
  g_mutex_unlock (*locker);
}

G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(GMutexLocker, g_mutex_locker_clear)

Created attachment 296167 [details] [review]
Add GMutexLocker

Lars and I chatted about this during FOSDEM as well and here are a few comments about the proposed approach.

We should either have GMutexLocker be a proper type or not.  If it is a proper type then it ought to be unconditionally defined, with a normal _init and _clear function.  The other alternative is that we stop pretending and just define a special syntax specifically for locks.

When discussing it, I think we came to the conclusion that a pointer-based API would be nicer (and also theorised about the name GMutexHolder).

GMutexHolder *
g_mutex_holder_new (GMutex *mutex)
{
  g_mutex_lock (mutex);
  return (GMutexHolder *) mutex;
}

void
g_mutex_holder_free (GMutexHolder *holder)
{
  g_mutex_unlock ((GMutex *) holder);
}


this way we could use g_autoptr():

  g_autoptr(GMutexHolder) *lock = g_mutex_holder_new (&lock);

without relying on either

  a) a non-standard return-struct-by-value-but-really-is-actually-a-pointer _init function; or

  b) a separate _init line


I'd really like to hear input from the Vala developers about which approach they think would better suit use from that language.

I came to the same conclusion, that's exactly my last patch, with différent names.

(In reply to comment #9)
> I came to the same conclusion, that's exactly my last patch, with différent
> names.

It's not, and I talked about the reasons why:

 - you only define it on GNUC

 - the non-standard _init function is weird

 - the lack of a proper _clear function is also weird

 - g_auto() vs. g_autoptr() and _init/_clear vs. _new/_free


These may seem like some pretty pedantic things to argue about, but conventions are very important in GLib.

(In reply to comment #10)
>  - you only define it on GNUC

I don't see a usecase for it without g_autoptr() which is not defined outside of GNUC, afaik. But as you wish, doesn't change anything for me.

>  - the non-standard _init function is weird

Included that as "with different names". I agree new/free is better here.

>  - the lack of a proper _clear function is also weird

At first I though it would be pointless since the whole point is to clear with g_autoptr magic. But you're right it's better to have complete API, and it would also allow nice early-unlock with g_clear_pointer(&locker, g_mutex_locker_free); and relock with locker = g_mutex_locker_new(&muter);

>  - g_auto() vs. g_autoptr() and _init/_clear vs. _new/_free

My proposed patch, in contrary to what I said before, does use g_autoptr().

Created attachment 296201 [details] [review]
Add GMutexLocker

I just have one doubt: _free() funcs are usually NULL-safe. Should it be changed to: if(locker)g_mutex_unlock((GMutex*)locker); or rename it to _clear(), or it's fine line that?

Review of attachment 296201 [details] [review]:

Looks nice, modulo the name (but I don't care _that_ much) and some minor docs issues.

I do still want to wait to hear that this is nicely usable from Vala, however.

::: glib/gthread.h
@@ +281,3 @@
+ * Lock @mutex and return a new #GMutexLocker. Unlock with
+ * g_mutex_locker_free(). Using g_mutex_lock() or g_mutex_unlock() on @mutex
+ * while a #GMutexLocker exists leads to undefined behaviour.

This is not technically true.  g_mutex_lock() [from another thread] on a mutex with an existing GMutexLocker is perfectly well defined: it blocks until the lock is released.

@@ +286,3 @@
+ * |[
+ * {
+ *   static GMutex mutex;

move the mutex definition out of the function, perhaps?

this is the more usual case...

(In reply to comment #13)
> I just have one doubt: _free() funcs are usually NULL-safe. Should it be
> changed to: if(locker)g_mutex_unlock((GMutex*)locker); or rename it to
> _clear(), or it's fine line that?

_free functions are not usually NULL safe.

(In reply to comment #14)
> Review of attachment 296201 [details] [review]:
> 
> Looks nice, modulo the name (but I don't care _that_ much) and some minor docs
> issues.

I don't care about locker/holder neither. I used locker because I've seen that in Qt API.

> I do still want to wait to hear that this is nicely usable from Vala, however.

Ok

> ::: glib/gthread.h
> @@ +281,3 @@
> + * Lock @mutex and return a new #GMutexLocker. Unlock with
> + * g_mutex_locker_free(). Using g_mutex_lock() or g_mutex_unlock() on @mutex
> + * while a #GMutexLocker exists leads to undefined behaviour.
> 
> This is not technically true.  g_mutex_lock() [from another thread] on a mutex
> with an existing GMutexLocker is perfectly well defined: it blocks until the
> lock is released.

Right, it's only doing g_mutex_unlock(&mutex) that can lead to undefined behaviour when the auto cleanup will call g_mutex_locker_free(). Just wanted to state that it's better not to play with the mutex behind locker's back within the same thread.

> @@ +286,3 @@
> + * |[
> + * {
> + *   static GMutex mutex;
> 
> move the mutex definition out of the function, perhaps?
> 
> this is the more usual case...

ok

At a first glance it should be clean to use in vala. It would behave like any
other non-refcount object created (i.e. a Compact class), and which vala will
free automatically.
We won't use g_auto of course.

Talking about free(), what about using a refcount instead?

Created attachment 296208 [details] [review]
Add GMutexLocker

Can we have a macro like this instead of having to do the whole g_autoptr assignment dance every time?

#define G_LOCK_FOR_SCOPE(mutex) g_autoptr(GMutexLocker) G_PASTE(mutex,locker) = g_mutex_locker_new (mutex);

How about just adding a method on the various synchronization primitives that return a pointer instead of gboolean? Then we can just define the release function to unlock the mutex. No new structs, etc.

{
  g_autoptr(GMutex) mutex = g_mutex_acquire (&my_mutex);
}

G_PASTE(mutex,locker) won't work if mutex is "self->mutex". But some kind of macro to make it shorted could be nice, I agree.

What about:

#define G_DECLARE_LOCKER(name, mutex) g_autoptr(GMenuLocker) name = g_mutex_locker_new(mutex);

(In reply to comment #20)
> How about just adding a method on the various synchronization primitives that
> return a pointer instead of gboolean? Then we can just define the release
> function to unlock the mutex. No new structs, etc.
> 
> {
>   g_autoptr(GMutex) mutex = g_mutex_acquire (&my_mutex);
> }

In the end, that's exactly what the proposed patch does, but with different naming. IMO having a specific typedef make it more explicit.

Code pushed as wip/xclaesse/locker

(In reply to comment #20)
> How about just adding a method on the various synchronization primitives that
> return a pointer instead of gboolean? Then we can just define the release
> function to unlock the mutex. No new structs, etc.
> 
> {
>   g_autoptr(GMutex) mutex = g_mutex_acquire (&my_mutex);
> }

I don't care for this because I consider it to be extremely surprising that the cleanup function for GMutex would be _unlock and not _free.

Attachment 296208 [details] pushed as 1404d3e - Add GMutexLocker

I pushed your patch with two minor changes:

 - moved the autoptr define to the separate header

 - added a warning about portability to the docs

Hi,

I have opened up Bug 744190 as the " Add GMutexLocker" patch broke builds on compilers which did not have "inline" in C-mode.

With blessings, thank you!