I do this a slightly different way:

See:

https://git.gnome.org/browse/libgsystem/tree/src/libgsystem.h#n28

Here's an example of a function which uses it:

https://git.gnome.org/browse/ostree/tree/src/libostree/ostree-repo-commit.c#n1599

This works particularly well with the local allocation macros, but doesn't depend on them.  Basically you have an additional intermediate local variable, and only set the out value when done.

Colin: Unless I've misunderstood I think your example covers a slightly different use case to mine.  You use-case covers returning values from a function via out-arguments whereas mine is about passing objects to a function which take ownership of those objects.

You could argue some overlap in that you could use g_move_object in your implementation of gs_transfer_out_value to save one line of code but I think the use-cases are mostly separate.

I do like your technique of only setting the out arguments at the end.  I like to use that transactional style myself.  It's also useful if locking is required as you can hold locks for the shortest possible time over operations that cannot fail.

Review of attachment 293921 [details] [review]:

I'm sorry to say that I don't think this very much.

From the description, I though that this would be a macro like g_set_object() that took the reference handed to it (but still handled cleaning up the existing reference in the variable).  I would probably even have said 'no' to that, and it would still be more generally useful than this.

Functions that (transfer-full) their 'in' arguments are not common and I don't really want to make them any easier to use.  Sorry :(

Fair enough.  I find myself needing this kind of thing fairly regularly when interacting with GStreamer, but come to think of it that's mostly when passing types like GstBuffer, GstCaps, GstEvent, etc. which are all GstMiniObjects rather than GObjects.

I suppose transfer-full is more natural with a streaming-like API like in GStreamer where objects move down the pipeline rather than more traditional APIs with more usual call and return semantics.

It's probably also because in a performance-sensitive library like gstreamer, doing constant atomic refcount ops is very noticeable, where with transfer-in-full you don't touch the refcount.

For the record, I think this is a reasonable idea, but not for the use-case of passing values to (transfer full) parameters (I agree with Ryan here — that’s a pretty niche use-case).

I think it would be useful for Colin’s (transfer full) return value use-case, but also for variable assignments which transfer ownership, e.g.:

    GObject *found_object = NULL;  /* owned */

    foreach (i in some_container until found_object != NULL) {
        GObject *element = wrap_element_and_return_ref (i);

        if (some_predicate (element)) {
            found_object = g_move_object (&element);
        }

        g_clear_object (&element);
    }

It’s a bit of a contrived example, but I have come across cases in the real world where it would be useful, typically where a variable is updated on one code path but not on another (and the code paths aren’t trivial).

That all said, this can easily be handled using g_object_ref() for all use cases except high-performance ones, as Ryan says. So g_move_object() really isn’t general purpose enough to be in GLib. I guess it could usefully be in GStreamer as gst_mini_object_move() though, or something like that.

Reopening this because of comment 6.  Now that we have g_autoptr() this functionality suddenly seems extremely useful.

I'd prefer if we called it g_take_pointer(), though.  It has nothing to do with GObject in any way and 'take' is the name we typically use for reference-stealing in our platform.

Created attachment 296263 [details] [review]
Add new API g_take_pointer()

This is particularly nice when used with g_autoptr().  See examples in
the docs.

Created attachment 296264 [details] [review]
tests: add a test case for g_take_pointer()

Just some basic checking to make sure it works as intended.

(In reply to comment #7)
> I'd prefer if we called it g_take_pointer(), though.  It has nothing to do with
> GObject in any way and 'take' is the name we typically use for
> reference-stealing in our platform.

Isn't it more like steal than take? As far as I can see, foo_take_bar() is normally

    /**
     * foo_take_bar:
     * @bar: (transfer full): a Bar
     *
     * Set foo's contents to bar, taking ownership.
     */
    void foo_take_bar (Foo *self, Bar *bar);

whereas foo_steal_bar() is normally more like

    /**
     * foo_steal_bar:
     *
     * Remove and return foo's contents.
     *
     * Returns: (transfer full): foo's bar
     */
    Bar *foo_steal_bar (Foo *self);

(although I see from Gst docs that they sometimes use "take" for both models.)

Review of attachment 296263 [details] [review]:

::: glib/gmem.h
@@ +178,3 @@
+ */
+static inline gpointer
+g_take_pointer (gpointer pp)

Shouldn’t that be (g_take_pointer) to prevent macro expansion? I realise the macro is defined later, but probably best to be safe.

@@ +191,3 @@
+/* type safety */
+#define g_take_pointer(pp) \
+  (0 ? (*(pp)) : g_take_pointer(pp))

And here too.

Review of attachment 296264 [details] [review]:

It would be good to see some tests of the non-macro version too. See /object/set and /object/set-function in gobject/tests/reference.c.

(In reply to comment #10)
> (In reply to comment #7)
> > I'd prefer if we called it g_take_pointer(), though.

> Isn't it more like steal than take?

To further muddy the naming waters, it's basically the same operation as g_task_propagate_pointer() (where "propagate" came from g_propagate_error(), although that doesn't NULL out the original unfortunately). There was some reason I didn't use "steal" there, though I don't remember what it was. (Maybe it was just because "steal_int" and "steal_boolean" sounded silly.)

(In reply to comment #11)
> Review of attachment 296263 [details] [review]:
> 
> ::: glib/gmem.h
> @@ +178,3 @@
> + */
> +static inline gpointer
> +g_take_pointer (gpointer pp)
> 
> Shouldn’t that be (g_take_pointer) to prevent macro expansion? I realise the
> macro is defined later, but probably best to be safe.

Ya.  Okay.

> 
> @@ +191,3 @@
> +/* type safety */
> +#define g_take_pointer(pp) \
> +  (0 ? (*(pp)) : g_take_pointer(pp))
> 
> And here too.

It's really not required here on account of being inside of the macro expansion of the same name, but it doesn't hurt either.

(In reply to comment #13)
> > Isn't it more like steal than take?
> 
> To further muddy the naming waters, it's basically the same operation as
> g_task_propagate_pointer() (where "propagate" came from g_propagate_error(),
> although that doesn't NULL out the original unfortunately). There was some
> reason I didn't use "steal" there, though I don't remember what it was. (Maybe
> it was just because "steal_int" and "steal_boolean" sounded silly.)

If I had to pick then I'd say "steal" is the more stetic of the two.

Created attachment 296274 [details] [review]
Add new API g_steal_pointer()

This is particularly nice when used with g_autoptr().  See examples in
the docs.

This patch is based upon an idea (and original patch submission) from
Will Manley <will@williammanley.net>.

Created attachment 296275 [details] [review]
tests: add a test case for g_steal_pointer()

Just some basic checking to make sure it works as intended.

Attachment 296274 [details] pushed as e668796 - Add new API g_steal_pointer()
Attachment 296275 [details] pushed as aa68b3d - tests: add a test case for g_steal_pointer()

Who reviewed the this patch before commit?  Or are you counting it as an update of someone else's patch submission?

I am counting it as a trivial update (ie: renaming and adding of extra parens) of the patches submitted in comment 8 and comment 9 and reviewed by Dan and Philip.

Sorry -- forgot to mention Simon as well.

Ok, that's reasonable, thanks.

Hi,

I have opened up Bug 744190 as the "Add new API g_steal_pointer()" broke builds on compilers which did not have "inline" in C-mode.

With blessings, thank you!

Comment on attachment 296274 [details] [review]
Add new API g_steal_pointer()

Is the g_steal_pointer() macro type-safe? The code snippet

    int a = 1;
    int *b = &a;
    float *c;

    c = g_steal_pointer (&b);

causes no warnings from gcc (gcc (GCC) 8.1.1 20180502 (Red Hat 8.1.1-1)). The type-safe macro for g_object_ref() uses typeof(), and https://bugzilla.gnome.org/show_bug.cgi?id=790697#c37 noted that the '0 ?...' alternative fails to raise warnings for incompatible pointer types.

The typeof version is:

#define g_steal_pointer(pp) ((__typeof__(*pp)) (g_steal_pointer) (pp))

and does correctly warn in the above case. It's a compiler extension, but glib already uses it in g_object_ref().

(In reply to Peter Bloomfield from comment #23)
> Comment on attachment 296274 [details] [review] [review]
> Add new API g_steal_pointer()
> 
> Is the g_steal_pointer() macro type-safe? The code snippet
> 
>     int a = 1;
>     int *b = &a;
>     float *c;
> 
>     c = g_steal_pointer (&b);
> 
> causes no warnings from gcc (gcc (GCC) 8.1.1 20180502 (Red Hat 8.1.1-1)).
> The type-safe macro for g_object_ref() uses typeof(), and
> https://bugzilla.gnome.org/show_bug.cgi?id=790697#c37 noted that the '0
> ?...' alternative fails to raise warnings for incompatible pointer types.
> 
> The typeof version is:
> 
> #define g_steal_pointer(pp) ((__typeof__(*pp)) (g_steal_pointer) (pp))
> 
> and does correctly warn in the above case. It's a compiler extension, but
> glib already uses it in g_object_ref().

This seems like it would be a good improvement. Could you please file a new bug report about it? If you could attach a tested patch, that would get things moving a lot faster, since we’re currently quite strained on development resources. Thanks!

https://bugzilla.gnome.org/show_bug.cgi?id=796341