GNOME Bugzilla – Bug 749527
add weak pointer helpers similar to g_set_object
Last modified: 2017-12-21 10:05:01 UTC
Bug 741589 added g_set_object() which is very convenient for setters because of the pattern: if (g_set_object (...)) g_object_notify (...) Having this same semantic for weak pointers is also useful. We use this pattern heavily in Builder since we deal with weak pointers all over the place. It's not pretty but, they have worked well for us for the last few months. If this is something that is relatively interesting, I'll make a formal patch for glib. #define ide_clear_weak_pointer(ptr) \ (*(ptr) ? (g_object_remove_weak_pointer((GObject*)*(ptr), (gpointer*)ptr),*(ptr)=NULL,1) : 0) #define ide_set_weak_pointer(ptr,obj) \ ((obj!=*(ptr))?(ide_clear_weak_pointer(ptr),*(ptr)=obj,((obj)?g_object_add_weak_pointer((GObject*)obj,(gpointer*)ptr),NULL:NULL),1):0)
I should note that the semantic is the same. Our code looks like: if (ide_set_weak_pointer (&self->foo, foo)) g_object_notify (self, "foo"); And in dispose/finalize: ide_clear_weak_pointer (&self->foo);
Yes, I think that would be a good idea. Patch welcome.
Created attachment 365645 [details] [review] Helpers implementations + unit tests First attempt at introducing g_set_weak_pointer() & g_clear_weak_pointer(). Implementations are the same as their strong reference (g_set_object() & g_clear_object()) counterparts: * g_clear_weak_pointer() is an extern function plus a macro. * g_set_weak_pointer() is an inline function plus a macro. Simple unit tests provided. Feedback and review more than welcome.
Review of attachment 365645 [details] [review]: ::: gobject/gobject.c @@ +3393,3 @@ + * Since: 2.56 + **/ +#undef g_clear_weak_pointer Instead of #undef, do: void (g_clear_weak_pointer) (gpionter *weak_pointer_location)
Created attachment 365716 [details] [review] Helpers implementations + unit tests, no #unref Right. g_clear_object() function definition should probably be modified also, though.
Is it true these days that we can guarantee our supported builds have support for "static inline"? I know at one point that wasn't true, but I'm not sure any more.
(In reply to Christian Hergert from comment #6) > Is it true these days that we can guarantee our supported builds have > support for "static inline"? I know at one point that wasn't true, but I'm > not sure any more. Yes: https://wiki.gnome.org/Projects/GLib/CompilerRequirements#Support_for_.27static_inline.27
Review of attachment 365716 [details] [review]: The new functions need to be listed in docs/reference/gobject/gobject-sections.txt so they appear in the documentation. ::: gobject/gobject.c @@ +3392,3 @@ + * + * Since: 2.56 + **/ Nitpick: Please terminate gtk-doc comments with `*/` not `**/`. The latter is deprecated syntax. @@ +3394,3 @@ + **/ +void +(g_clear_weak_pointer) (gpointer *weak_pointer_location) Why isn’t this one static inline? ::: gobject/gobject.h @@ +759,3 @@ + * + * Updates a pointer to weakly refer to @new_object. It assigns @new_object + * to @weak_pointer_location and ensure that @weak_pointer_location will s/ensure/ensures/ ::: gobject/tests/reference.c @@ +321,3 @@ + + g_clear_weak_pointer (&weak); + g_assert (weak == NULL); You can use g_assert_null (weak) so this test continues to work when compiling with assertions disabled. (Same in various other places.) @@ +366,3 @@ + gpointer weak = NULL; + + g_assert (!g_set_weak_pointer (&weak, NULL)); You can use g_assert_false() here instead of g_assert(). (Same in various other places.) @@ +770,3 @@ g_test_add_func ("/object/initially-unowned", test_initially_unowned); g_test_add_func ("/object/weak-pointer", test_weak_pointer); + g_test_add_func ("/object/weak-pointer-clear", test_weak_pointer_clear); Make all of these test paths have an `/object/weak-pointer` group in them, so that running `./reference -p /object/weak-pointer` will run *all* the weak pointer tests. i.e. Path `/object/weak-pointer/clear` rather than `/object/weak-pointer-clear`, and the same for the other tests below.
(In reply to Philip Withnall from comment #8) > @@ +3394,3 @@ > + **/ > +void > +(g_clear_weak_pointer) (gpointer *weak_pointer_location) > > Why isn’t this one static inline? For no reasons, except that g_clear_object() isn't static inline either. I don't really understand with g_clear_object() was not declare as such in the first place, but as I could definitely miss something here, I preferred mimicking existing code firstly. Should we go for static inline?
(In reply to Martin Blanchard from comment #9) > (In reply to Philip Withnall from comment #8) > > Why isn’t this one static inline? > > For no reasons, except that g_clear_object() isn't static inline either. I > don't really understand with g_clear_object() was not declare as such in the > first place, but as I could definitely miss something here, I preferred > mimicking existing code firstly. Should we go for static inline? Yeah, go with static inline so that less magic happens in the preprocessor. Bug #674634 is where g_clear_object() was last rewritten, and the bug doesn’t mention anything about static inline, so I suspect it was just not considered (rather than considered and rejected).
Created attachment 365823 [details] [review] Helpers implementations, static inline
Review of attachment 365823 [details] [review]: Great, thanks.
Pushed to master.