Created attachment 171247 [details] [review]
Remove tests testing input transfer!=none

Created attachment 171248 [details] [review]
transfer for inout specifies only out-direction, in is always transfer=none

Review of attachment 171247 [details] [review]:

Looks good.

Review of attachment 171248 [details] [review]:

Also looks good, thanks!

# Bug 630788 - [tests] avoid input arguments with transfer!=none. - UNCONFIRMED

(In reply to comment #0)
> Since it is not recommended for functions to take ownership of input arguments,
> testsuite should not do it either.

Wait wait wait. "Not recommended" is different from "not allowed". The scanner currently implements transfer-full in arguments, and so it's proper for the regression test suite to test that it's doing it correctly so that you don't end up shipping a release in which that functionality is broken. OTOH, if you don't care that you might ship a future release in which in-transfer-full is broken, then you ought to just remove that functionality from the scanner now.

(In reply to comment #6)
> (In reply to comment #0)
> > Since it is not recommended for functions to take ownership of input arguments,
> > testsuite should not do it either.
> 
> Wait wait wait. "Not recommended" is different from "not allowed".

I think of it as "deprecated", and I'll remove it very soon actually.

I think it is resolved

(In reply to comment #6)
> (In reply to comment #0)
> > Since it is not recommended for functions to take ownership of input arguments,
> > testsuite should not do it either.
> 
> Wait wait wait. "Not recommended" is different from "not allowed". The scanner
> currently implements transfer-full in arguments, and so it's proper for the
> regression test suite to test that it's doing it correctly so that you don't
> end up shipping a release in which that functionality is broken. OTOH, if you
> don't care that you might ship a future release in which in-transfer-full is
> broken, then you ought to just remove that functionality from the scanner now.

FWIW, I'd even disagree on "not recommended" - it's sometimes _nice_ in C to be able to do this and one example is the return()-family of functions in GDBusMethodInvocation, for example

 http://developer.gnome.org/gio/unstable/GDBusMethodInvocation.html#g-dbus-method-invocation-return-value

The alternative is to use floating references but that "solution" comes with its own set of problems.

(In reply to comment #9)
> FWIW, I'd even disagree on "not recommended" - it's sometimes _nice_ in C to be
> able to do this and one example is the return()-family of functions in
> GDBusMethodInvocation, for example
> 
> 
> http://developer.gnome.org/gio/unstable/GDBusMethodInvocation.html#g-dbus-method-invocation-return-value
> 
> The alternative is to use floating references but that "solution" comes with
> its own set of problems.

Ouch... I personally believe that APIs consuming its input parameters are broken, but it is just my personal opinion.  But even when we would allow transfer!=none on IN arguments, there is still question what to do with INOUT arguments; in order to support them, there would have to be 2 transfer modes, 1st for IN direction and 2nd for OUT direction. Another, half-hearted option would be to define that for INOUT args, transfer mode for IN direction is always NONE and for OUT direction it is governed by 'transfer' annotation, but it starts looking like a mess.

I'd still favor to disallow transfer!=none for input arguments, decision is on GI maintainers.

This is not resolved; we are no longer testing container/full-transfer input arguments, but they are still scanned and output without warnings.

(In reply to comment #11)
> This is not resolved; we are no longer testing container/full-transfer input
> arguments, but they are still scanned and output without warnings.

So would it be OK to implement warning in the scanner when input argument with container/full-transfer is detected?  I can prepare such patch, but I'd like to know if it has a chance to be accepted.

g_thread_join() is another example of such an API (although not a GObject one).

g_bytes_unref_to_array(), g_bytes_unref_to_data() are others (again not GObject).

I think consuming arguments is definitely pretty reasonable in some cases, although best avoided if at all possible.  We can't just drop support for it entirely...

There's also lots of API in GStreamer that takes ownership of the input parameters for several reasons, including efficency.

And then there's stuff like gtk_container_add() that also takes ownership of the input parameter.

(In reply to comment #14)
> And then there's stuff like gtk_container_add() that also takes ownership of
> the input parameter.

Given how floating refs work now in GI, gtk_container_add() does *not* take ownership of the input parameter, it internally performs (equivalent of) g_object_ref_sink().

If floating refs worked in the way you propose in bug #657202, then gtk_container_add() might need some special (transfer) treatment.  But given how it works now, there is no special thing needed, (transfer none) works just fine.

(In reply to comment #14)
> And then there's stuff like gtk_container_add() that also takes ownership of
> the input parameter.

Just to add to the comments from Pavel, the usual thinking with floating refs is not that the first call takes the reference away from you, but rather that you never owned it to begin with.

Ok, right, my mistake :) But that's only distracting from the original issue here and should be discussed in #657202 then (and I still don't think transfer-none is correct for "stealing" of floating refs).

(In reply to comment #17)
> Ok, right, my mistake :) But that's only distracting from the original issue
> here and should be discussed in #657202 then 

Agreed.  So lets get back ontopic.  There are 2 major problems I know about with (transfer!=none) on input arguments.  Both are related to the typelib-consumers (i.e. typically language binding implementations):

1) It is not clear what to do when transfer!=none is used on inout argument; does it mean (in:transfer=full)(out:transfer=full)?  What if there is an API which consumes argument as input and sets output on the same argument with transfer=none?

2) Even when GI scanner and typelibs itself do support transfer attribute on input arguments, AFAIK most of current bindings have issues with supporting it correctly, varying from ignoring transfer on input arguments to trying to support it a failing in some corner cases - I tried to support it in my binding implementation, and it turned out that it is surprisingly complex and hard, although it does not seem so at the first glance.

This means that to solve this bug correctly, we need to:

1) specify exactly what (transfer) attribute means on inout arguments
2) make existing bindings obey transfer attribute on in/inout arguments

Yes, I would propose to add specific in:transfer and out:transfer annotations for the case when this differs. If an in-out argument is transfer-full only I would assume that it steals ownership of the input parameter and gives you ownership of the output parameter (in fact I currently can't see a use-case for doing something different).

(In reply to comment #18)
> What if there is an API
> which consumes argument as input and sets output on the same argument with
> transfer=none?

Then it's not introspectable and you have to mark it (skip). It's also just bad API.

> I tried to support it in my binding
> implementation, and it turned out that it is surprisingly complex and hard,
> although it does not seem so at the first glance.

No, all you have to do is "copy" the argument and pass the copy to the function rather than the original (where "copy" means the same thing you do when receiving a (transfer none) return value. eg, g_object_ref() or g_boxed_copy())

(In reply to comment #20)
> > I tried to support it in my binding
> > implementation, and it turned out that it is surprisingly complex and hard,
> > although it does not seem so at the first glance.
> 
> No, all you have to do is "copy" the argument and pass the copy to the function
> rather than the original (where "copy" means the same thing you do when
> receiving a (transfer none) return value. eg, g_object_ref() or g_boxed_copy())

Sure, but it is not 'all you have to do', this is just 'simpler half of what you have to do'.  The more complex half is error handling, i.e. undoing the copies when marshaling of some other argument goes wrong (e.g. bad type of argument passed) before you are able to invoke the function.

Of course, it is doable, but brings much more complications to already very complex function marshaling, when written in a way that it does not slow down normal error-less pass.

(At that point I asked at #introspection and suggestion was to just not bother with input arguments with transfer, that it is not supported, so that's why I originally created this bug and patches.)

(In reply to comment #18)
> 1) It is not clear what to do when transfer!=none is used on inout argument;
> does it mean (in:transfer=full)(out:transfer=full)?  What if there is an API
> which consumes argument as input and sets output on the same argument with
> transfer=none?

My favorite puzzle of the last two days is annotating the parameters of g_iconv() in a way that makes sense. (transfer none) for both inout pointers-to-arrays is in there, because: 1) g-ir-scanner slaps it by default onto the inout integers, and I wanted all my ducks in a row; 2) it makes more sense in the context.

But perhaps g_iconv() just has to be annotated (skip) for being too weird, and a new function grown alongside it specifically for bindings, with separate in and out parameters.

Speaking generally, (inout) (transfer none) could mean "the callee does not take ownership the value, but may mutate the passed reference"
and (inout) (transfer full) could be "the callee consumes the input value and puts out another value for the caller to own, though it may give back the input value as a shortcut". Are there other cases in practical use?

Some types for which (inout) (transfer none) appears to make sense:

1. Primitive values: ownership transfer is irrelevant, so "none" is as good as anything else.

2. Arrays and strings: what gets passed in is a slice (pointer + length) or a pointer to a null-terminated array. With (inout), the slice itself is passed by reference. With no ownership transfer, there is no requirement that the slice corresponds to an entire allocated buffer, so array subslices can be efficiently used by some bindings. On the out, the slice may be modified, presumably keeping within the bounds of the original slice. The binding then may use its language's concept of an array slice for the modified slice, copying it out of the input value in the worst case.

3. Collections where the "subslice" metaphor from 2 can be applied, meaning primarily vectors and lists.

(In reply to comment #22)
> 
> But perhaps g_iconv() just has to be annotated (skip) for being too weird, and
> a new function grown alongside it specifically for bindings, with separate in
> and out parameters.

Yes.

(In reply to comment #23)
> > But perhaps g_iconv() just has to be annotated (skip) for being too weird, and
> > a new function grown alongside it specifically for bindings, with separate in
> > and out parameters.
> 
> Yes.

Here you are, bug #737978.

[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gobject-introspection/issues/37.