Created attachment 147270 [details] [review]
Fix GI_TYPE_TAG_VOID type checking

ATM, NULL is passed for void arguments, so it makes sense to require None to be passed.

How would you marshal a void argument then?

Perhaps we can detect the type of the Python object being passed and do our best to fill the GArgument correctly. Not sure how doable it is.

(In reply to comment #3)
> Perhaps we can detect the type of the Python object being passed and do our
> best to fill the GArgument correctly. Not sure how doable it is.

Yeah, wonder what do the other bindings.

The set_data_* wrappers in PyGObject just pass the pointer to the PyGObject as the gpointer. I don't think we need to worry too much for these cases as will be rare.

Created attachment 147480 [details] [review]
Add support for Any arguments

Review of attachment 147480 [details] [review]:

::: gi/pygi-argument.c
@@ +620,3 @@
         case GI_TYPE_TAG_VOID:
     switch (type_tag) {
+            arg.v_pointer = object;

What about ownership transfers?

@@ +1275,3 @@
+                object = arg->v_pointer;
+                Py_INCREF(arg->v_pointer);
+            if (is_pointer) {

Is the non-pointer case useful?

::: tests/libtestgi.c
@@ +2735,3 @@
+ * @closure: (transfer none):
+ * test_gi_gclosure_in:
+/**

The annotation doesn't match the function name.

::: tests/libtestgi.h
@@ +450,3 @@
+gpointer test_gi_any_in_return (gpointer any);
+
+/* Any */

Rename it test_gi_pointer_in_return (gpointer pointer) to keep consistency.

Is it possible to split the test: one for the return value, one for the input value?

::: tests/test_gi.py
@@ +1018,3 @@
+        object1 = TestGI.Object(int=42)
+    def test_any(self):
+class TestAny(unittest.TestCase):

Use a simple number so that we don't depend on all the GObject stuff.

Created attachment 148274 [details] [review]
Add support for Any arguments

(In reply to comment #7)
> Review of attachment 147480 [details] [review]:
> 
> ::: gi/pygi-argument.c
> @@ +620,3 @@
>          case GI_TYPE_TAG_VOID:
>      switch (type_tag) {
> +            arg.v_pointer = object;
> 
> What about ownership transfers?

Don't know what we could do if not GI_TRANSFER_NOTHING, so have added a warning in that case.

> @@ +1275,3 @@
> +                object = arg->v_pointer;
> +                Py_INCREF(arg->v_pointer);
> +            if (is_pointer) {
> 
> Is the non-pointer case useful?

Yes, for the void case (!is_pointer) we need to return None.

> Is it possible to split the test: one for the return value, one for the input
> value?

Don't see how, as we are passing a PyObject here, the C side would need to test it with CPython, which I guess we don't want in TestGI.

> ::: tests/test_gi.py
> @@ +1018,3 @@
> +        object1 = TestGI.Object(int=42)
> +    def test_any(self):
> +class TestAny(unittest.TestCase):
> 
> Use a simple number so that we don't depend on all the GObject stuff.

You meant as in TestGI.Object.new(42)? Done, but why?

Review of attachment 148274 [details] [review]:

::: gi/pygi-argument.c
@@ +1278,3 @@
+                object = arg->v_pointer;
+                Py_INCREF(arg->v_pointer);
+            if (is_pointer) {

Do only one Py_INCREF(object) instead of both Py_INCREF.

Perhaps a comment like "Assume the pointer is a Python object" would be useful for future readings, I think.

And a warning if the transfer is not none?

::: tests/libtestgi.c
@@ +2728,3 @@
 }
+test_gi_pointer_in_return (gpointer any)
+gpointer

I'd call the argument 'pointer' instead of 'any', to keep consistency with the other tests.

::: tests/test_gi.py
@@ +1022,3 @@
+        object1 = TestGI.Object.new(42)
+    def test_any(self):
+class TestAny(unittest.TestCase):

I think I already wrote that in my review of a previous patch: using a simple Python integer is enough; no need to use a GObject:

self.assertEquals(TestGI.pointer_in_return(42), 42)

Created attachment 148279 [details] [review]
Add support for Any arguments

Review of attachment 148279 [details] [review]:

::: tests/test_gi.py
@@ +1021,3 @@
+        self.assertEquals(TestGI.pointer_in_return(42), 42)
+    def test_any(self):
+class TestAny(unittest.TestCase):

I suggest those relabellings:
- TestAny -> TestPointer
- test_any -> test_pointer_in_return

Created attachment 148281 [details] [review]
Add support for Any arguments

Review of attachment 148281 [details] [review]:

Good. Thanks!

Review of attachment 148281 [details] [review]:

Good. Thanks!

The following fix has been pushed:
fad89e1 Add support for Any arguments

Created attachment 148282 [details] [review]
Add support for Any arguments