After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 741589 - gobject: Add g_set_object() convenience function to set GObject pointers
gobject: Add g_set_object() convenience function to set GObject pointers
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gobject
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2014-12-16 11:32 UTC by Philip Withnall
Modified: 2014-12-18 13:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
docs: Remove a mention of g_clear_object() being atomic (995 bytes, patch)
2014-12-16 11:32 UTC, Philip Withnall
committed Details | Review
gobject: Add g_set_object() convenience function to set GObject pointers (5.60 KB, patch)
2014-12-16 11:32 UTC, Philip Withnall
needs-work Details | Review
gobject: Add g_set_object() convenience function to set GObject pointers (6.06 KB, patch)
2014-12-16 18:04 UTC, Philip Withnall
needs-work Details | Review
gobject: Add g_set_object() convenience function to set GObject pointers (5.96 KB, patch)
2014-12-17 09:34 UTC, Philip Withnall
needs-work Details | Review
gobject: Add g_set_object() convenience function to set GObject pointers (6.19 KB, patch)
2014-12-18 11:39 UTC, Philip Withnall
committed Details | Review

Description Philip Withnall 2014-12-16 11:32:43 UTC
Add a g_set_object() convenience function similar in style to g_clear_object(). See the commit message for it for details.
Comment 1 Philip Withnall 2014-12-16 11:32:45 UTC
Created attachment 292798 [details] [review]
docs: Remove a mention of g_clear_object() being atomic

It is no longer atomic.
Comment 2 Philip Withnall 2014-12-16 11:32:53 UTC
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.
Comment 3 Allison Karlitskaya (desrt) 2014-12-16 14:38:25 UTC
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.
Comment 4 Allison Karlitskaya (desrt) 2014-12-16 14:39:04 UTC
Review of attachment 292798 [details] [review]:

Of course.
Comment 5 A. Walton 2014-12-16 15:32:36 UTC
(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 6 Philip Withnall 2014-12-16 17:11:31 UTC
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
Comment 7 Philip Withnall 2014-12-16 18:04:10 UTC
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.
Comment 8 Allison Karlitskaya (desrt) 2014-12-16 18:47:31 UTC
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.
Comment 9 Philip Withnall 2014-12-17 09:34:58 UTC
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.
Comment 10 Philip Withnall 2014-12-17 09:36:42 UTC
(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.
Comment 11 Allison Karlitskaya (desrt) 2014-12-17 16:49:14 UTC
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.
Comment 12 Xavier Claessens 2014-12-17 17:18:37 UTC
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.
Comment 13 Xavier Claessens 2014-12-17 17:28:52 UTC
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.
Comment 14 Xavier Claessens 2014-12-17 17:35:36 UTC
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");
}
Comment 15 Allison Karlitskaya (desrt) 2014-12-17 23:19:07 UTC
(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 :)
Comment 16 Philip Withnall 2014-12-18 11:39:51 UTC
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.
Comment 17 Philip Withnall 2014-12-18 11:41:31 UTC
(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.
Comment 18 Allison Karlitskaya (desrt) 2014-12-18 13:02:53 UTC
Review of attachment 292971 [details] [review]:

Looks good now.  Thanks for all the efforts.
Comment 19 Allison Karlitskaya (desrt) 2014-12-18 13:03:07 UTC
Review of attachment 292971 [details] [review]:

(sorry -- wrong status)
Comment 20 Philip Withnall 2014-12-18 13:57:41 UTC
Great, thanks for the thorough reviews.

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