Created attachment 360889 [details] [review]
gmain: add g_clear_source API

It's a very common pattern to see code that looks like this in
dispose() or finalize() implementations:

if (priv->source_id > 0)
  {
    g_source_remove (priv->source_id);
    priv->source_id = 0;
  }

This API allows to accomplish the same goal with a single line:

g_clear_source (&priv->source_id);

Review of attachment 360889 [details] [review]:

I wonder if we should perhaps generalise this to g_clear_int(), so that it could also be used for things like g_bus_unwatch_name() and g_bus_unown_name(). I’m very keen for something which allows GSource IDs to be cleared to go into GLib though.

::: glib/gmain.c
@@ +2422,3 @@
+ * @tag_ptr must not be %NULL.
+ *
+ * If the ID is less or equal than zero then this function does nothing.

guints can’t be less than zero.

@@ +2429,3 @@
+ *
+ * Since: 2.56
+ **/

Nitpick: Ending a gtk-doc comment with `**/` is deprecated. Just use `*/`.

Matthias, what do you think?

Created attachment 360921 [details] [review]
gmain: add g_clear_source API

--

Updated for your comment.

About a more generic g_clear_uint() (I don't think g_clear_int() would be correct, since source IDs and return values from g_bus_own_name() are unsigned), I thought about it but had decided to first get feedback on the idea.
I would not be opposed to making this more generic and take a function pointer if that's the consensus.

(In reply to Cosimo Cecchi from comment #4)
> About a more generic g_clear_uint() (I don't think g_clear_int() would be
> correct, since source IDs and return values from g_bus_own_name() are
> unsigned), I thought about it but had decided to first get feedback on the
> idea.
> I would not be opposed to making this more generic and take a function
> pointer if that's the consensus.

You’re right, it should be unsigned. I think we should indeed go with g_clear_uint() rather than g_clear_source().

(In reply to Philip Withnall from comment #3)
> Matthias, what do you think?

Seems fine to me. I think I like the clear_source variant better than an overly generic clear_uint

(In reply to Matthias Clasen from comment #6)
> (In reply to Philip Withnall from comment #3)
> > Matthias, what do you think?
> 
> Seems fine to me. I think I like the clear_source variant better than an
> overly generic clear_uint

I agree, I'd rather have a g_clear_source().

Unless, of course, you want a generic:

typedef void (* GClearUIntFunc) (guint value);

static inline void
g_clear_uint (gpointer pp, GClearUIntFunc func)
{
  guint *value_p = pp;
  guint value = *value_p;

  if (value != 0)
    {
      if (func != NULL)
        func (value);
      *value_p = 0;
    }
}

and then:

#define g_clear_source(id) \
  g_clear_uint ((id), (GClearUIntFunc) g_source_remove)

But I think we're over-engineering this.

I’m happy to go with g_clear_source() if that’s what the consensus is. It just means that we’re going to continue to be stuck with:

if (bus_watch_id > 0)
  {
    g_bus_unwatch_name (bus_watch_id);
    bus_watch_id = 0;
  }

(In reply to Philip Withnall from comment #8)
> I’m happy to go with g_clear_source() if that’s what the consensus is. It
> just means that we’re going to continue to be stuck with:
> 
> if (bus_watch_id > 0)
>   {
>     g_bus_unwatch_name (bus_watch_id);
>     bus_watch_id = 0;
>   }

Then we could really add a `g_clear_handle_id()` and define `g_clear_source()` and `g_clear_bus_watch()`.

(In reply to Emmanuele Bassi (:ebassi) from comment #9)
> (In reply to Philip Withnall from comment #8)
> > I’m happy to go with g_clear_source() if that’s what the consensus is. It
> > just means that we’re going to continue to be stuck with:
> > 
> > if (bus_watch_id > 0)
> >   {
> >     g_bus_unwatch_name (bus_watch_id);
> >     bus_watch_id = 0;
> >   }
> 
> Then we could really add a `g_clear_handle_id()` and define
> `g_clear_source()` and `g_clear_bus_watch()`.

Certainly we could, but that seems like an unnecessary APIsplosion given that they all follow the same pattern.

Created attachment 361035 [details] [review]
Turn g_clear_source() into a generic function

We still want a GSource specific g_clear_source(), but since we have
multiple variants of the "unsigned value as a handle id that needs to
be cleared" in GLib, we should provide a generic implementation as the
basis for specialisation.

Created attachment 361036 [details] [review]
docs: Add new g_clear_* symbols

Created attachment 361037 [details] [review]
Add g_clear_bus_watch_name()

Use g_clear_handle_id() to implement a clear function for the bus name
watching handle.

I hate to bring it up, but signal ids are longs...

(In reply to Matthias Clasen from comment #14)
> I hate to bring it up, but signal ids are longs...

They’re also only unique when paired with an object instance, so are totally not suitable for clearing with g_clear_handle_id(), and hence haven’t been mentioned so far.

---

Overall, I’m in favour of just adding g_clear_handle_id(). However, if others think it’s worthwhile to add convenience wrappers around it for GSources/bus names/bus watches/whatever, I’m not going to argue against it. I just think that’s adding a few too many new APIs, given that g_clear_handle_id() would be simple enough to use inline in such cases.

I'm okay with landing g_clear_handle_id(). If people want to golf it, and introduce g_clear_source() as a separate, incremental improvement, is also fine with me.

OK, let’s go with g_clear_handle_id() for now and then see if the need arises for some convenience wrappers for sources, bus ownership and bus name watches.

Cosimo, can you rework the patches to add g_clear_handle_id()/GClearHandleFunc as the first commit in the series please?

Created attachment 362941 [details] [review]
gmain: add g_clear_handle_id API

It's a very common pattern to see code that looks like this in
dispose() or finalize() implementations:

if (priv->source_id > 0)
  {
    g_source_remove (priv->source_id);
    priv->source_id = 0;
  }

This API allows to accomplish the same goal with a single line:

g_clear_handle_id (&priv->source_id, (GClearHandleFunc) g_source_remove);

Thanks to Emmanuele Bassi <ebassi@gnome.org> for making the patch
generic.

--

Updated and squashed patch for g_clear_handle_id().

Review of attachment 362941 [details] [review]:

A few minor comments, but this looks good to me otherwise.

::: glib/gmain.c
@@ +2416,3 @@
+/**
+ * g_clear_handle_id: (skip)
+ * @tag_ptr: (not nullable): a pointer to the handler id

s/id/ID/

@@ +2419,3 @@
+ * @clear_func: (not nullable): the function to call to clear the handler
+ *
+ * Clears a numeric handler, such as a GSource id.

s/GSource id/#GSource ID/

@@ +2423,3 @@
+ * @tag_ptr must be a valid pointer to the variable holding the handler.
+ *
+ * If the id is zero then this function does nothing.

s/id/ID/

@@ +2424,3 @@
+ *
+ * If the id is zero then this function does nothing.
+ * Otherwise, clear_func() is called with the id as a parameter, and the tag is

s/id/ID/

@@ +2425,3 @@
+ * If the id is zero then this function does nothing.
+ * Otherwise, clear_func() is called with the id as a parameter, and the tag is
+ * set to 0.

s/0/zero/ to be consistent with above (or change above to `0`).

::: glib/gmain.h
@@ +566,3 @@
+/**
+ * GClearHandleFunc:
+ * @handle_id: the handle id to clear

s/id/ID/

@@ +570,3 @@
+ * Specifies the type of function passed to g_clear_handle_id().
+ * The implementation is expected to free the resource identified
+ * by @handle_id; for instance, if @handle_id is a #GSource id,

s/id/ID/

@@ +585,3 @@
+    G_STATIC_ASSERT (sizeof *(tag_ptr) == sizeof (guint)); \
+    guint *_tag_ptr = (guint *) (tag_ptr);                 \
+    guint tag;                                             \

Should we use a less likely to collide variable name than `tag`, given that this is a macro?

Created attachment 362985 [details] [review]
Updated for review comments.

Review of attachment 362985 [details] [review]:

Looks good to push with these two changes.

::: glib/gmain.c
@@ +2419,3 @@
+ * @clear_func: (not nullable): the function to call to clear the handler
+ *
+ * Clears a numeric handler, such as a GSource ID.

#GSource

::: glib/gmain.h
@@ +585,3 @@
+    G_STATIC_ASSERT (sizeof *(tag_ptr) == sizeof (guint)); \
+    guint *_tag_ptr = (guint *) (tag_ptr);                 \
+    guint handle_id;                                       \

That seems just as likely to collide as tag, given that people could quite legitimately call this as:

handle_id = g_timeout_add (…);
g_clear_handle_id (&handle_id, g_source_remove);

I’d prefix it with an underscore.

Review of attachment 362985 [details] [review]:

::: glib/gmain.h
@@ +585,3 @@
+    G_STATIC_ASSERT (sizeof *(tag_ptr) == sizeof (guint)); \
+    guint *_tag_ptr = (guint *) (tag_ptr);                 \
+    guint handle_id;                                       \

There's also `__COUNTER__`; we should likely make use of that more.  See `git grep __COUNTER__` in current source.

Created attachment 363076 [details] [review]
gmain: add g_clear_handle_id API

--

Updated for review comments. I decided not to use __COUNTER__ here yet because that did not seem portable. We can always clean that up later.

Review of attachment 363076 [details] [review]:

Great, thanks for the iterations.

Attachment 363076 [details] pushed as 5ebd8f6 - gmain: add g_clear_handle_id API