Created attachment 211680 [details] [review]
gicallableinfo: Add two new convenience methods: is_method and can_throw_gerror

Created attachment 211681 [details] [review]
girffi: Fix g_callable_info_prepare_closure for certain callables

Namely, those that are methods and those that throw GErrors.
We have very similar code in two places that calculate arg lengths and
argument types to stick into libffi. Merge, clean up, and correct both.

Review of attachment 211680 [details] [review]:

::: girepository/gicallableinfo.c
@@ +86,3 @@
+ * @info: a #GICallableInfo
+ *
+ * Can this #GICallableInfo throw a GError?

Returns: %TRUE if this #GICallableInfo can throw a GError
Since: 1.34

@@ +117,3 @@
+ * @info: a #GICallableInfo
+ *
+ * Is the callable info a method? For #GIVFuncInfo<!-- -->s,

Determine if the callable info a method.  For #GIVFuncInfo<!-- -->s,

@@ +125,3 @@
+ * matches the number of arguments in the raw C method. For methods, there
+ * is one more C argument than is exposed by introspection: the "self"
+ * or "this" object.

Since: 1.34

@@ +142,3 @@
+    return TRUE;
+  case GI_INFO_TYPE_CALLBACK:
+    return FALSE;

this should be TRUE, right?  At least the corresponding code in girffi.c set is_method=True for callbacks.

Review of attachment 211681 [details] [review]:

Hmm...can you give me an example of a C API that was broken?

This does look like a nice code cleanup if it works, but this code is very subtle.  Do the pygobject tests work with this patch?

I'll look at this more in detail in a bit.

g_callable_info_prep_closure didn't handle methods or throws.

pygobject tests all pass.

The issue with callbacks being methods is that it assumes that it should chew up the first method argument (or something like that), which isn't how signal handlers behave in gjs/pygobject right now.

(In reply to comment #6)
> pygobject tests all pass.
> 
> The issue with callbacks being methods is that it assumes that it should chew
> up the first method argument (or something like that), which isn't how signal
> handlers behave in gjs/pygobject right now.

I guess what confuses me is it seems very likely that pygobject would be working around this, and this patch would break their workaround.

$ git grep g_callable_info_prepare_closure
gi/pygi-callbacks.c:        destroy_notify->closure = g_callable_info_prepare_closure ( (GICallableInfo*) glib_destroy_notify,
gi/pygi-closure.c:        g_callable_info_prepare_closure (info, &closure->cif, _pygi_closure_handle,
$

Is PyGObject missing tests for these cases?

I'd like to see these patches go in as well.  They still apply cleanly to master, and they fix the problems I'm seeing.  Specifically, they make sure that the implicit instance argument for signals and vfuncs is provided for.  For vfuncs, you can work around the limitations in the current code by passing the interface info of the associated struct field to g_callable_info_prepare_closure.  For signals, this is not possible.

As far as I can tell, pygobject uses this workaround for vfuncs.  For signals, they seem to be using a basic custom marshaller that does not use g_callable_info_prepare_closure at all.  So I think pygobject should not be affected by this change.

I agree with Torsten, if he thinks it's fit to go in it can. If it affects pyobject (which seems unlikely) they should just fix their workarounds instead of affecting g-i.

+1

Review of attachment 211680 [details] [review]:

::: girepository/gicallableinfo.c
@@ +142,3 @@
+    return TRUE;
+  case GI_INFO_TYPE_CALLBACK:
+    return FALSE;

I'm quite sure this is a mistake.

Created attachment 221770 [details] [review]
gicallableinfo: Add two new convenience methods: is_method and can_throw_gerror

Created attachment 221771 [details] [review]
girffi: Fix g_callable_info_prepare_closure for certain callables

Namely, those that are methods and those that throw GErrors.
We have very similar code in two places that calculate arg lengths and
argument types to stick into libffi. Merge, clean up, and correct both.

(In reply to comment #11)
> Review of attachment 211680 [details] [review]:
> 
> ::: girepository/gicallableinfo.c
> @@ +142,3 @@
> +    return TRUE;
> +  case GI_INFO_TYPE_CALLBACK:
> +    return FALSE;
> 
> I'm quite sure this is a mistake.

This comment was quite confusing to me at first, so let me clarify: Jasper is saying, and I agree, that the original code in girffi.c that sets is_method = TRUE for GI_INFO_TYPE_CALLBACK, is mistaken.  is_method = TRUE is a shorthand for "has an implicit instance argument".  As far as I can see, callbacks always have all arguments explicitly listed, including the instance.

Right, that's what I'm saying. Maybe I should add a documentation comment to get_is_method?

(also, I replied with Splinter, which doesn't actually generate a proper comment reply which quotes the original, it just quotes the exact same line snippet. I think the Bugzilla people fixed this when they rewrote Splinter and integrated it into BZ4)

(In reply to comment #15)
> Right, that's what I'm saying. Maybe I should add a documentation comment to
> get_is_method?

Yeah, and I would also suggest to split the bug fix (changing from is_method=TRUE to is_method=FALSE for callbacks) off into its own commit.  That way it is not hidden inside a bigger refactoring commit.

Review of attachment 221770 [details] [review]:

Ok.

Review of attachment 221771 [details] [review]:

This does look like a good cleanup.

Attachment 221770 [details] pushed as d220bb7 - gicallableinfo: Add two new convenience methods: is_method and can_throw_gerror
Attachment 221771 [details] pushed as d8970fb - girffi: Fix g_callable_info_prepare_closure for certain callables

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