GNOME Bugzilla – Bug 617780
Add support for out args in callbacks
Last modified: 2010-05-24 15:44:19 UTC
.
Created attachment 160355 [details] [review] Add support for out args in callbacks
Comment on attachment 160355 [details] [review] Add support for out args in callbacks >From 2ba363377e58f5c87441d2aebc826ef493324466 Mon Sep 17 00:00:00 2001 >From: Tomeu Vizoso <tomeu@sugarlabs.org> >Date: Wed, 5 May 2010 18:22:39 +0200 >Subject: [PATCH] Add support for out args in callbacks > >https://bugzilla.gnome.org/show_bug.cgi?id=617780 >--- > gi/pygi-closure.c | 303 +++++++++++++++++++++++++++++++++++++++------------- > tests/test_gi.py | 4 + > 2 files changed, 231 insertions(+), 76 deletions(-) > >diff --git a/gi/pygi-closure.c b/gi/pygi-closure.c >index 2c8d0b3..cb4c1a7 100644 >--- a/gi/pygi-closure.c >+++ b/gi/pygi-closure.c . . . >+ >+static void >+_pygi_closure_convert_arguments (GICallableInfo *callable_info, void **args, >+ void *user_data, PyObject **py_args, >+ GArgument **out_args) >+{ >+ int n_args = g_callable_info_get_n_args (callable_info); >+ int n_in_args = 0; >+ int n_out_args = 0; >+ int i; >+ GArgument *g_args = NULL; >+ >+ *py_args = PyTuple_New (n_args); >+ *out_args = g_new0 (GArgument, n_args); >+ g_args = _pygi_closure_convert_ffi_arguments (callable_info, args); >+ >+ for (i = 0; i < n_args; i++) { >+ GIArgInfo *arg_info = g_callable_info_get_arg (callable_info, i); >+ GIDirection direction = g_arg_info_get_direction(arg_info); >+ >+ if (direction == GI_DIRECTION_IN || direction == GI_DIRECTION_INOUT) { >+ GITypeInfo *arg_type = g_arg_info_get_type (arg_info); >+ GITypeTag arg_tag = g_type_info_get_tag(arg_type); >+ GITransfer transfer = g_arg_info_get_ownership_transfer (arg_info); >+ PyObject *value; >+ GArgument *arg; >+ >+ if (direction == GI_DIRECTION_IN && arg_tag == GI_TYPE_TAG_VOID && >+ g_type_info_is_pointer(arg_type)) { >+ >+ if (user_data == NULL) { >+ Py_INCREF(Py_None); >+ value = Py_None; >+ } else { >+ Py_INCREF(user_data); >+ value = user_data; >+ } >+ } else { >+ if (direction == GI_DIRECTION_IN) >+ arg = (GArgument*) &g_args[i]; >+ else >+ arg = (GArgument*) g_args[i].v_pointer; >+ >+ value = _pygi_argument_to_object (arg, arg_type, transfer); >+ } >+ PyTuple_SET_ITEM (*py_args, n_in_args, value); >+ n_in_args++; >+ } >+ >+ if (direction == GI_DIRECTION_OUT || direction == GI_DIRECTION_INOUT) { >+ (*out_args)[n_out_args] = g_args[i]; >+ n_out_args++; >+ } >+ } Above looks like it fixes the ref issue I fixed in my conflicting patch. Bellow you need to return an error code on failure or check for NULL in _pygi_closure_handle >+ _PyTuple_Resize (py_args, n_in_args); >+} . . . > void > _pygi_closure_handle (ffi_cif *cif, > void *result, >@@ -35,20 +254,12 @@ _pygi_closure_handle (ffi_cif *cif, > { > PyGILState_STATE state; > PyGICClosure *closure = data; >- gint n_args, i; >- GIArgInfo *arg_info; >- GIDirection arg_direction; >- GITypeInfo *arg_type; >- GITransfer arg_transfer; >- GITypeTag arg_tag; > GITypeTag return_tag; > GITransfer return_transfer; > GITypeInfo *return_type; > PyObject *retval; > PyObject *py_args; >- PyObject *pyarg; >- gint n_in_args, n_out_args; >- >+ GArgument *out_args; > > /* Lock the GIL as we are coming into this code without the lock and we > may be executing python code */ >@@ -58,72 +269,11 @@ _pygi_closure_handle (ffi_cif *cif, > return_tag = g_type_info_get_tag(return_type); > return_transfer = g_callable_info_get_caller_owns(closure->info); > >- n_args = g_callable_info_get_n_args (closure->info); >- >- py_args = PyTuple_New(n_args); >- if (py_args == NULL) { >- PyErr_Print(); >- goto end; >- } >- >- n_in_args = 0; >- >- for (i = 0; i < n_args; i++) { >- arg_info = g_callable_info_get_arg (closure->info, i); >- arg_type = g_arg_info_get_type (arg_info); >- arg_transfer = g_arg_info_get_ownership_transfer(arg_info); >- arg_tag = g_type_info_get_tag(arg_type); >- arg_direction = g_arg_info_get_direction(arg_info); >- switch (arg_tag){ >- case GI_TYPE_TAG_VOID: >- { >- if (g_type_info_is_pointer(arg_type)) { >- if (closure->user_data == NULL) { >- Py_INCREF(Py_None); >- if(PyTuple_SetItem(py_args, n_in_args, Py_None) != 0) { >- PyErr_Clear(); >- goto end; >- } >- } else if (PyTuple_SetItem(py_args, n_in_args, closure->user_data) != 0) { >- PyErr_Print(); >- goto end; >- } >- >- Py_XINCREF(closure->user_data); >- >- n_in_args++; >- continue; >- } >- } >- case GI_TYPE_TAG_ERROR: >- { >- continue; >- } >- default: >- { >- pyarg = _pygi_argument_to_object (args[i], >- arg_type, >- arg_transfer); >- >- if(PyTuple_SetItem(py_args, n_in_args, pyarg) != 0) { >- PyErr_Print(); >- goto end; >- } >- n_in_args++; >- g_base_info_unref((GIBaseInfo*)arg_info); >- g_base_info_unref((GIBaseInfo*)arg_type); >- } >- } >- >- } >- >- if(_PyTuple_Resize (&py_args, n_in_args) != 0) { >- PyErr_Print(); >- goto end; >- } >- >- retval = PyObject_CallObject((PyObject *)closure->function, py_args); >+ _pygi_closure_convert_arguments ((GICallableInfo *)closure->info, args, >+ closure->user_data, >+ &py_args, &out_args); You should add this: if(py_args == NULL) goto end >+ retval = PyObject_CallObject ((PyObject *)closure->function, py_args); > Py_DECREF(py_args); > > if (retval == NULL) { >@@ -131,15 +281,16 @@ _pygi_closure_handle (ffi_cif *cif, > goto end; > } > >- *(GArgument*)result = _pygi_argument_from_object(retval, return_type, return_transfer); >+ _pygi_closure_set_out_arguments (closure->info, retval, out_args, result); > > end: > g_base_info_unref((GIBaseInfo*)return_type); > > PyGILState_Release(state); > >- if (closure->user_data) First off why would you need to check for NULL when using Py_XDECREF (it ignores NULL values)? Secondly the decref is an error. The user_data is decreffed when the tuple is decreffed. You do need to add a decref to _pygi_invoke_closure_free. I can add that to my patch though since I need to write a test case. Can you fix the issues I outlined in your patch? Thanks. >+ if (closure->user_data) { > Py_XDECREF(closure->user_data); >+ } > > /* Now that the closure has finished we can make a decision about how > to free it. Scope call gets free'd at the end of wrap_g_function_info_invoke >diff --git a/tests/test_gi.py b/tests/test_gi.py >index 3db562d..1fc500b 100644 >--- a/tests/test_gi.py >+++ b/tests/test_gi.py >@@ -1409,6 +1409,9 @@ class TestPythonGObject(unittest.TestCase): > def do_method_int8_in(self, int8): > self.val = int8 > >+ def do_method_int8_out(self): >+ return 42 >+ > def do_method_with_default_implementation(self, int8): > self.props.int = int8 * 2 > >@@ -1425,6 +1428,7 @@ class TestPythonGObject(unittest.TestCase): > object_ = self.Object(int = 42) > object_.method_int8_in(84) > self.assertEqual(object_.val, 84) >+ self.assertEqual(object_.method_int8_out(), 42) > > object_.method_with_default_implementation(42) > self.assertEqual(object_.val, 84) >-- >1.6.6.1
Review of attachment 160355 [details] [review]: ::: gi/pygi-closure.c @@ +172,3 @@ + } else { + Py_INCREF(user_data); + value = user_data; user_data is void*. You may want to cast it to PyObject* before calling Py_INCREF, or incref value instead. pygi-closure.c: In function ‘_pygi_closure_convert_arguments’: pygi-closure.c:173: warning: dereferencing ‘void *’ pointer pygi-closure.c:173: error: request for member ‘ob_refcnt’ in something not a structure or union
Created attachment 161720 [details] [review] Add support for out args in callbacks This patch refactors argument marshalling for closures in preparation for more complete support. Addresses the review comments.
Review of attachment 161720 [details] [review]: Looks good, passes tests. Even passes my refcount test. Good to commit.
Review of attachment 161720 [details] [review]: Actually can you also add that this fixes userdata refcounting?
Review of attachment 161720 [details] [review]: I meant, add that to the commit comments if I wasn't clear
Attachment 161720 [details] pushed as 8eb8094 - Add support for out args in callbacks