Created attachment 276006 [details] [review]
Parse and expose ownership transfer for instance parameters

Review of attachment 276006 [details] [review]:

In general, the implied instance argument is hard to deal with on the bindings side. PyGI determines if there is an instance argument by checking if the info type is GI_INFO_TYPE_FUNCTION and the flags have GI_FUNCTION_IS_METHOD or the info type is GI_INFO_TYPE_VFUNC (not sure about signals). So I think also having an API like g_callable_info_has_instance_arg() to handle those details would be nice, otherwise they are encoded in the bindings. Without a clear conditional context for whether or not the callable has an instance argument, g_callable_info_get_instance_ownership_transfer() doesn't make a lot of sense to me.

Ideally it would be nice to have an API like g_callable_info_get_instance_argument() which gives back a full blown GIArgInfo with all the bits filled out correctly...

How about some tests? :)

::: girepository/gicallableinfo.c
@@ +298,3 @@
+      FunctionBlob *blob;
+      blob = (FunctionBlob *)&rinfo->typelib->data[rinfo->offset];
+{

Since blob->instance_transfer_ownership being set means transfer-full, perhaps the struct attribute should be named accordingly: blob->instance_transfer_full ?

::: girepository/gitypelib-internal.h
@@ +558,2 @@
   guint16 is_static   : 1;
+  guint16 instance_transfer_ownership : 1;

Another option is to add the flag to the SignatureBlob which is available for all the callable blobs and will simplify the implementation. I did this for the last patch in bug 729543.

(In reply to comment #2)
> Review of attachment 276006 [details] [review]:
> 
> In general, the implied instance argument is hard to deal with on the bindings
> side. PyGI determines if there is an instance argument by checking if the info
> type is GI_INFO_TYPE_FUNCTION and the flags have GI_FUNCTION_IS_METHOD or the
> info type is GI_INFO_TYPE_VFUNC (not sure about signals). So I think also
> having an API like g_callable_info_has_instance_arg() to handle those details
> would be nice, otherwise they are encoded in the bindings. Without a clear
> conditional context for whether or not the callable has an instance argument,
> g_callable_info_get_instance_ownership_transfer() doesn't make a lot of sense
> to me.

Well, it's not like that will change any time soon: vfuncs, methods and signals have an instance argument, everything else doesn't. It's basic OOP really, not even g-i conventions.
 
> ::: girepository/gicallableinfo.c
> @@ +298,3 @@
> +      FunctionBlob *blob;
> +      blob = (FunctionBlob *)&rinfo->typelib->data[rinfo->offset];
> +{
> 
> Since blob->instance_transfer_ownership being set means transfer-full, perhaps
> the struct attribute should be named accordingly: blob->instance_transfer_full
> ?

It's called transfer_ownership to mean transfer_full in other places too, and transfer_full is g-ir-scanner terminology, not g-ir-compiler/libgirepository (it would be transfer_everything)

> ::: girepository/gitypelib-internal.h
> @@ +558,2 @@
>    guint16 is_static   : 1;
> +  guint16 instance_transfer_ownership : 1;
> 
> Another option is to add the flag to the SignatureBlob which is available for
> all the callable blobs and will simplify the implementation. I did this for the
> last patch in bug 729543.

Yeah, that's a good idea.

Created attachment 278489 [details] [review]
Parse and expose ownership transfer for instance parameters

Knowing the ownership transfer for instance parameters is
necessary for correct memory management of functions which
"eat" their instance argument, such as g_dbus_method_invocation_return_*.
Parse this information from the gir file and store in the
typelib, and then provide new API on GICallableInfo to
retrieve this.

Review of attachment 278489 [details] [review]:

Looks good, thanks for adding the tests.

(In reply to comment #3)
> Well, it's not like that will change any time soon: vfuncs, methods and signals
> have an instance argument, everything else doesn't. It's basic OOP really, not
> even g-i conventions.

I guess my complaint is the API seems like a Liskov violation or at least closely related (bug 709456 is probably a better example). The problem is we have the base type GICallableInfo describing a method which doesn't make sense for certain sub-types, and even returns a valid value for them. From a readability POV, it just seems strange to have to type check a base class in order to verify if methods defined on the base type are valid for the sub-type.

Trying to rework this is obviously not practical, so the suggestion was to package up the sub-type knowledge in an additional method (which will be useful in other contexts as well). I'm happy to add a patch for something like g_callable_info_has_instance_arg() if others agree...

> > ::: girepository/gicallableinfo.c
> > @@ +298,3 @@
> > +      FunctionBlob *blob;
> > +      blob = (FunctionBlob *)&rinfo->typelib->data[rinfo->offset];
> > +{
> > 
> > Since blob->instance_transfer_ownership being set means transfer-full, perhaps
> > the struct attribute should be named accordingly: blob->instance_transfer_full
> > ?
> 
> It's called transfer_ownership to mean transfer_full in other places too, and
> transfer_full is g-ir-scanner terminology, not g-ir-compiler/libgirepository
> (it would be transfer_everything)

Cool, thanks for the explanation.

It's true that you can't really use g_callable_info_get_instance_ownership_transfer()
without knowledge of the subclasses. But that is also true of, say, g_callable_info_invoke(),
and the alternative would be to duplicate this API across the three subclasses.
Besides, I don't believe any new subclass will be introduced in the future, so
this is really a minor issue.

Attachment 278489 [details] pushed as a4c9d09 - Parse and expose ownership transfer for instance parameters

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