GNOME Bugzilla – Bug 603598
Add basic support for unions
Last modified: 2010-04-23 14:33:42 UTC
Just treating them the same as other structs for now.
Created attachment 148911 [details] [review] Add basic support for unions
Created attachment 148913 [details] [review] Add basic support for unions
Review of attachment 148913 [details] [review]: ::: gi/pygi-argument.c @@ +1870,3 @@ + PyErr_Format(PyExc_ValueError, "couldn't find a wrapper for type '%s'", + if (!PyErr_Occurred()) + g_type_name(type)); This is better than simply overwriting the error. However, I propose to simply break here, and to solve bug #605215 (set the error in _pygi_type_get_from_g_type) I just created.
Review of attachment 148913 [details] [review]: Mostly style & error issues looked at. ::: gi/module.py @@ +35,3 @@ StructInfo, \ ConstantInfo, \ + UnionInfo, \ Can we use () here instead of \ @@ +127,3 @@ metaclass = GObjectMeta bases = (gobject.GInterface,) + elif isinstance(info, StructInfo) or isinstance(info, UnionInfo): isinstance(info, [StructInfo, UnionInfo]) ::: gi/pygi-argument.c @@ +1868,3 @@ if (py_type == NULL) { py_type = _pygi_type_get_from_g_type(type); + if (!PyErr_Occurred()) I guess this should go into _pygi_type_get_from_g_type() OR you should re-raise it if you can provide any additional information to the exception message here. ::: gi/pygi-boxed.c @@ +68,1 @@ if (info == NULL) { Ditto about re-raising exception @@ +82,3 @@ + size = g_union_info_get_size((GIUnionInfo *)info); + case GI_INFO_TYPE_UNION: + switch (g_base_info_get_type(info)) { include the type of the sent in message in the error message ::: gi/pygi-info.c @@ +2075,3 @@ + if (py_info == NULL) { + Py_CLEAR(infos); + break; Shouldn't this be an error? @@ +2110,3 @@ + + if (py_info == NULL) { + Py_CLEAR(infos); Ditto, see above ::: tests/libtestgi.c @@ +3154,2 @@ } +TestGIUnion * Can you push this to everything instead? It's okay to assert the values. It should be documented as well, using gtk-doc docstrings so when know the intention of the test
Review of attachment 148913 [details] [review]: Err, I forgot to have a look at the tests. ::: tests/libtestgi.c @@ +3179,3 @@ + (GBoxedCopyFunc) test_gi_union_copy, +TestGIUnion * + (GBoxedFreeFunc) test_gi_union_free); I could be worth testing unions registered as pointers and unions not registered too. ::: tests/test_gi.py @@ +1337,3 @@ + def test_union_method(self): + def test_union(self): + struct = TestGI.Union() I guess union might be a better name than struct :-)
(In reply to comment #4) > Review of attachment 148913 [details] [review]: > ::: gi/module.py > @@ +35,3 @@ > StructInfo, \ > ConstantInfo, \ > + UnionInfo, \ > > Can we use () here instead of \ Yes, we could. But then it should be changed in all the modules. > ::: gi/pygi-info.c > @@ +2075,3 @@ > + if (py_info == NULL) { > + Py_CLEAR(infos); > + break; > > Shouldn't this be an error? _pygi_info_new sets the error, this sets infos to NULL and the function returns infos. > Can you push this to everything instead? The whole point of libtestgi is to have a complete set of tests for pygi. Please keep it complete, unless everything spans it entirely.
(In reply to comment #6) > > Can you push this to everything instead? > > The whole point of libtestgi is to have a complete set of tests for pygi. > Please keep it complete, unless everything spans it entirely. I just don't want that each binding duplicates all tests in everything.
Created attachment 150682 [details] [review] Add basic support for unions Comments addressed, but tests unchanged.
Review of attachment 150682 [details] [review]: OK.
Attachment 150682 [details] pushed as 3ba666b - Add basic support for unions