GNOME Bugzilla – Bug 622425
What should we do with NULL arrays
Last modified: 2010-06-23 17:11:57 UTC
pygobject was crashing in the tests when accessing a NULL (empty) array in a struct. As it turns out we were g_array_free'ing that array and it crashed in pygobject because it is compiled with the stricter abort on warnings flag. The first patch just checks for a NULL pointer before freeing. The second patch actually converts that NULL to an empty PyTuple instead of a PyNone when returning. The first patch fixes the issue and the second fixes the behavor assuming we wish to return an empty tuple instead of None. I personally think it is the right behavor but I don't want to commit without input from others.
Created attachment 164336 [details] [review] we shouldn't g_array_free NULL pointers
Created attachment 164337 [details] [review] return an empty tuple for NULL arrays instead of None * returns an empty tuple when a NULL array (empty array) is encountered * test the ability to send in both None and empty tuple for arrays * test the ability to send in both None and empty list for lists
Review of attachment 164336 [details] [review]: Looks wrong; ::: gi/pygi-info.c @@ -1211,3 @@ py_value = _pygi_argument_to_object (&value, field_type_info, GI_TRANSFER_NOTHING); - if ( (g_type_info_get_tag (field_type_info) == GI_TYPE_TAG_ARRAY) && Why not just if (value.v_pointer != NULL) ?
Review of attachment 164337 [details] [review]: Rest looks good. Should verify that it returns a list and not a tuple. ::: tests/test_everything.py @@ +98,3 @@ def test_in_nullable_array(self): Everything.test_array_int_null_in(None) + Everything.test_array_int_null_in(()) [] @@ +115,2 @@ def test_out_nullable_array(self): + self.assertEqual((), Everything.test_array_int_null_out()) [] ::: tests/test_gi.py @@ +1210,2 @@ self.assertEquals(0, struct.long_) + self.assertEquals((), struct.g_strv) [], not ()
(In reply to comment #4) > Review of attachment 164337 [details] [review]: > > Rest looks good. Should verify that it returns a list and not a tuple. > > ::: tests/test_everything.py > @@ +98,3 @@ > def test_in_nullable_array(self): > Everything.test_array_int_null_in(None) > + Everything.test_array_int_null_in(()) > > [] > > @@ +115,2 @@ > def test_out_nullable_array(self): > + self.assertEqual((), Everything.test_array_int_null_out()) > > [] > > ::: tests/test_gi.py > @@ +1210,2 @@ > self.assertEquals(0, struct.long_) > + self.assertEquals((), struct.g_strv) > > [], not () Arrays are returned as tuples in pygi.
(In reply to comment #3) > Review of attachment 164336 [details] [review]: > > Looks wrong; > > ::: gi/pygi-info.c > @@ -1211,3 @@ > py_value = _pygi_argument_to_object (&value, field_type_info, > GI_TRANSFER_NOTHING); > > - if ( (g_type_info_get_tag (field_type_info) == GI_TYPE_TAG_ARRAY) && > > Why not just if (value.v_pointer != NULL) ? Because it isn't necessarily an array. In PyGI we convert C arrays into GArrays and then send it to be converted to a PyObject. The GArray container then needs to be freed.
Comment on attachment 164336 [details] [review] we shouldn't g_array_free NULL pointers Pushed to pygobject
Created attachment 164406 [details] [review] return PyList instead of PyTuple for array, return empty list for NULL arrays * returns an empty list when a NULL array (empty array) is encountered * fix tests to check for lists instead of tuples or None * test the ability to send in both None and empty list for arrays and lists
Review of attachment 164406 [details] [review]: Excellent, looks great to me