Created attachment 292798 [details] [review]
docs: Remove a mention of g_clear_object() being atomic

It is no longer atomic.

Created attachment 292799 [details] [review]
gobject: Add g_set_object() convenience function to set GObject pointers

Along the same lines as g_clear_object(), g_set_object() is a
convenience function to update a GObject pointer, handling reference
counting transparently and correctly.

Specifically, it handles the case where a pointer is set to its current
value. If handled naïvely, that could result in the object instance
being finalised. In the following code, that happens when
(my_obj == new_value) and the object has a single reference:
    g_clear_object (&my_obj);
    my_obj = g_object_ref (new_value);

It also simplifies boilerplate code such as set_property()
implementations, which are otherwise long and boring.

Test cases included.

Review of attachment 292799 [details] [review]:

fwiw, I think it might be useful (since you check for this case anyway) to return TRUE/FALSE depending on if the value was already equal or not

That would allow for the following convenient style:

foo_set_bar (Foo *foo,
             Bar *bar)
{
  if (g_set_object (&foo->bar, bar))
    g_object_notify (foo, "bar");
}

::: gobject/gobject.c
@@ +3230,3 @@
+#undef g_set_object
+void
+g_set_object (GObject **object_ptr,

Please don't #undef like that -- it has the weird side effect that one half of the file gets to see the macro and the other half doesn't.

The better way is to write the function definition like so:

void
(g_set_object) (GObject **object_ptr, ...

which prevents the macro expansion.

::: gobject/gobject.h
@@ +655,3 @@
+void    g_set_object (GObject **object_ptr,
+                      GObject  *new_object);
+#define g_set_object(object_ptr, new_object) \

Why have a function form of this and then immediately after unconditionally define the inline version?


I'd also prefer some additional intelligence with respect to types here.  We should make sure that an assignment between the two would normally be allowed by the compiler.  That could easily be done by adding to the macro something like:

  if (0) { *obj_ptr = new_object }

which would be OK in terms of multiple uses of the parameters since it would never be evaluated.

Review of attachment 292798 [details] [review]:

Of course.

(In reply to comment #3)
> ::: gobject/gobject.h
> @@ +655,3 @@
> +void    g_set_object (GObject **object_ptr,
> +                      GObject  *new_object);
> +#define g_set_object(object_ptr, new_object) \
> 
> Why have a function form of this and then immediately after unconditionally
> define the inline version?
> 

While it probably doesn't matter as much here since I doubt object setting is going to be in the performance path... I'd rather have the macro expanded than deal with the penalty of the function call most of the time I'd use this trick, but I can't take a reference to a macro and pass that to another function when I need to.

But I'd also like it if the macro's body were identical to the function call it's replacing to prevent later debugging headaches. The trick I've used here is to expand the macro inside of the function such that the code is actually identical and you only have to write it once.

My 2c.

Comment on attachment 292798 [details] [review]
docs: Remove a mention of g_clear_object() being atomic

Attachment 292798 [details] pushed as e98a582 - docs: Remove a mention of g_clear_object() being atomic

Created attachment 292854 [details] [review]
gobject: Add g_set_object() convenience function to set GObject pointers

Along the same lines as g_clear_object(), g_set_object() is a
convenience function to update a GObject pointer, handling reference
counting transparently and correctly.

Specifically, it handles the case where a pointer is set to its current
value. If handled naïvely, that could result in the object instance
being finalised. In the following code, that happens when
(my_obj == new_value) and the object has a single reference:
    g_clear_object (&my_obj);
    my_obj = g_object_ref (new_value);

It also simplifies boilerplate code such as set_property()
implementations, which are otherwise long and boring.

Test cases included.

Review of attachment 292854 [details] [review]:

Looking better, but a few more comments.

::: gobject/gobject.h
@@ +653,3 @@
 
+/**
+ * g_set_object: (skip)

maybe move the docs to docs.c

@@ +691,3 @@
+
+  g_return_val_if_fail (object_ptr != NULL, FALSE);
+  g_return_val_if_fail (*object_ptr == NULL || G_IS_OBJECT (*object_ptr), FALSE);

really not a fan of all this extra checking, particularly considering g_object_ref() and g_object_unref() will do the same for us automatically

@@ +699,3 @@
+    g_object_unref (*object_ptr);
+
+  different = (*object_ptr != new_object);

would rather see the 'already equal' case handled as an early-return rather than doing a ref/unref for no reason at all.

Created attachment 292875 [details] [review]
gobject: Add g_set_object() convenience function to set GObject pointers

Along the same lines as g_clear_object(), g_set_object() is a
convenience function to update a GObject pointer, handling reference
counting transparently and correctly.

Specifically, it handles the case where a pointer is set to its current
value. If handled naïvely, that could result in the object instance
being finalised. In the following code, that happens when
(my_obj == new_value) and the object has a single reference:
    g_clear_object (&my_obj);
    my_obj = g_object_ref (new_value);

It also simplifies boilerplate code such as set_property()
implementations, which are otherwise long and boring.

Test cases included.

(In reply to comment #8)
> Review of attachment 292854 [details] [review]:
> 
> Looking better, but a few more comments.
> 
> ::: gobject/gobject.h
> @@ +653,3 @@
> 
> +/**
> + * g_set_object: (skip)
> 
> maybe move the docs to docs.c

There is no gobject/docs.c — only glib/docs.c. There are plenty of other docs in gobject.h, and I see no need to separate the doc comment from the function definition. That’s just asking for them to go out of sync.

> @@ +691,3 @@
> +
> +  g_return_val_if_fail (object_ptr != NULL, FALSE);
> +  g_return_val_if_fail (*object_ptr == NULL || G_IS_OBJECT (*object_ptr),
> FALSE);
> 
> really not a fan of all this extra checking, particularly considering
> g_object_ref() and g_object_unref() will do the same for us automatically

Fair enough. I removed the G_IS_OBJECT() checks, but have kept in the (object_ptr != NULL) check because nothing else does that.

> @@ +699,3 @@
> +    g_object_unref (*object_ptr);
> +
> +  different = (*object_ptr != new_object);
> 
> would rather see the 'already equal' case handled as an early-return rather
> than doing a ref/unref for no reason at all.

Good point. Fixed.

Review of attachment 292875 [details] [review]:

::: gobject/gobject.h
@@ +688,3 @@
+                GObject  *new_object)
+{
+  g_return_val_if_fail (object_ptr != NULL, FALSE);

I'd still prefer that you got rid of this.  It will harm performance and code size and it's actually useless in the 99% case where we will be using this.

Consider:

typedef struct {
  GObject parent_instance;

  Bar *bar;
} Foo;


...
  g_set_object (&foo->bar, bar);
...

in this case, the only real threat is that 'foo' was NULL.  foo->bar will end up as something like 0x20 or however many bytes offset into the struct it is.  I don't think this check is helpful.

Additionally, if object_ptr is NULL (or some offset to NULL) then everything is going to crash very very quickly, at which point the bug is going to be obvious.  It's also what would happen if anyone implemented this for themselves.

The check also adds a static string containing the failed expression as a message at each site of use (remember: this is inline).

@@ +706,3 @@
+#define g_set_object(object_ptr, new_object) \
+ (/* Check types match. */ \
+  0 ? *object_ptr = new_object, FALSE : \

You should add parens around the macro parameter expansions here.  Sorry for not catching that last time.

Since this is inspired from g_clear_object() then why not adding as well g_set_pointer() ?

gboolean g_set_pointer(gpointer *ptr, gpointer new_ptr, GDestroyNotify destroy, GBoxedCopyFunc copy);

g_set_object() would then be implemented as { return g_set_pointer(object_ptr, new_object, g_object_unref, g_object_ref); }

I use GBoxedCopyFunc here because it has the signature we want. GCopyFunc already exists but with an extra arg we probably don't need.

Maybe also g_set_boxed(gpointer *ptr, gpointer new_ptr, GType boxed_gtype) that would use g_boxed_copy/free(). But g_clear_boxed() was rejected because g_clear_pointer() was considered enough, so probably same here.

Hm, one potential confusion would be how we consider pointers equal then. For example we could want to avoid replacing if strings are equal. Maybe it needs an extra GCompareFunc argument, if NULL then it does pointer comparison.

A string property setter would look like:

void
foo_set_name (Foo *self, const gchar *new_name)
{
  if (g_set_pointer (&self->priv->name, new_name, g_strcmp0, g_strdup, g_free))
    g_object_notify (self, "name");
}

(In reply to comment #13)
> Hm, one potential confusion would be how we consider pointers equal then. For
> example we could want to avoid replacing if strings are equal. Maybe it needs
> an extra GCompareFunc argument, if NULL then it does pointer comparison.

I think you're building a pretty good case for not having this :)

Created attachment 292971 [details] [review]
gobject: Add g_set_object() convenience function to set GObject pointers

Along the same lines as g_clear_object(), g_set_object() is a
convenience function to update a GObject pointer, handling reference
counting transparently and correctly.

Specifically, it handles the case where a pointer is set to its current
value. If handled naïvely, that could result in the object instance
being finalised. In the following code, that happens when
(my_obj == new_value) and the object has a single reference:
    g_clear_object (&my_obj);
    my_obj = g_object_ref (new_value);

It also simplifies boilerplate code such as set_property()
implementations, which are otherwise long and boring.

Test cases included.

(In reply to comment #15)
> (In reply to comment #13)
> > Hm, one potential confusion would be how we consider pointers equal then. For
> > example we could want to avoid replacing if strings are equal. Maybe it needs
> > an extra GCompareFunc argument, if NULL then it does pointer comparison.
> 
> I think you're building a pretty good case for not having this :)

That does seem like an awful lot of callback confusion. At least with g_set_object() the two parameters are both intrinsically necessary.

(In reply to comment #11)
> Review of attachment 292875 [details] [review]:
> The check also adds a static string containing the failed expression as a
> message at each site of use (remember: this is inline).

Fair points. Removed.

> @@ +706,3 @@
> +#define g_set_object(object_ptr, new_object) \
> + (/* Check types match. */ \
> +  0 ? *object_ptr = new_object, FALSE : \
> 
> You should add parens around the macro parameter expansions here.  Sorry for
> not catching that last time.

Oooops. Fixed.

Review of attachment 292971 [details] [review]:

Looks good now.  Thanks for all the efforts.

Review of attachment 292971 [details] [review]:

(sorry -- wrong status)

Great, thanks for the thorough reviews.

Attachment 292971 [details] pushed as d951db4 - gobject: Add g_set_object() convenience function to set GObject pointers