GNOME Bugzilla – Bug 602735
Add a default constructor for boxed structures
Last modified: 2009-11-30 10:58:37 UTC
Allow boxed structures to be allocated (usually with g_new) if no default constructor is given.
Created attachment 148333 [details] [review] Set a default constructor for boxed structs that don't have one
Review of attachment 148333 [details] [review]: ::: gi/gimodule.c @@ +139,3 @@ +{ +_wrap_pyg_construct_boxed(PyObject *self, PyObject *args) +static PyObject * Use "O!" instead, with PyGIBaseInfo_Type to check py_info's type. @@ +158,3 @@ { "register_interface_info", (PyCFunction)_wrap_pyg_register_interface_info, METH_VARARGS }, { "set_object_has_new_constructor", (PyCFunction)_wrap_pyg_set_object_has_new_constructor, METH_VARARGS | METH_KEYWORDS }, + { "construct_boxed", (PyCFunction)_wrap_pyg_construct_boxed, METH_VARARGS }, And, as usual, I'd call it boxed_new, because of pyg_boxed_new :-p ::: tests/libtestgi.h @@ +562,1 @@ +void test_gi__boxed_with_constructor_struct_inout (TestGIBoxedWithConstructorStruct **struct_); Ouf, long. I'd keep those functions' prefix 'test_gi_boxed_struct_' instead.
Created attachment 148379 [details] [review] Set a default constructor for boxed structs that don't have one
Review of attachment 148379 [details] [review]: Pretty good! ::: gi/pygi-argument.c @@ +1526,3 @@ g_assert(is_pointer); + py_type = _pygi_type_get_from_g_type(type); + You'd better check the return value (this function could fail). ::: gi/pygi-boxed.c @@ +46,3 @@ +/* -*- Mode: C; c-basic-offset: 4 -*- + } + } Missing return statement? @@ +57,3 @@ + } + + ((PyGPointer *)self)->ob_type->tp_free((PyObject *)self); PyGPointer?
Created attachment 148585 [details] [review] Set a default constructor for boxed structs that don't have one
Created attachment 148586 [details] [review] Set a default constructor for boxed structs that don't have one
Review of attachment 148586 [details] [review]: Are there any differences with the patch I reviewed?!
Created attachment 148594 [details] [review] Set a default constructor for boxed structs that don't have one
(In reply to comment #7) > Review of attachment 148586 [details] [review]: > > Are there any differences with the patch I reviewed?! Sorry about that, here it is.
Review of attachment 148594 [details] [review]: ::: gi/pygi-argument.c @@ +1569,3 @@ + PyErr_Format(PyExc_ValueError, "couldn't find a wrapper for type '%s'", + py_type = _pygi_type_get_from_g_type(type); + py_type's refcount must be decremented afterward!
Created attachment 148643 [details] [review] Set a default constructor for boxed structs that don't have one
Review of attachment 148643 [details] [review]: ::: gi/pygi-boxed.c @@ +46,3 @@ + PyErr_Format(PyExc_TypeError, "missing introspection information"); + } + return; Hum, in this case it'll leak memory. I think it'd be better to output a warning and to jump to tp_free, even though it will not prevent it from leaking. If we really want to use a slice, I'd keep the size in a field of the C object structure. The deallocation function should never fail, and thus should not rely on the __info__ attribute that could be changed by the user.
Created attachment 148653 [details] [review] Set a default constructor for boxed structs that don't have one
(In reply to comment #12) > Review of attachment 148643 [details] [review]: > > ::: gi/pygi-boxed.c > @@ +46,3 @@ > + PyErr_Format(PyExc_TypeError, "missing introspection > information"); > + } > + return; > > Hum, in this case it'll leak memory. I think it'd be better to output a warning > and to jump to tp_free, even though it will not prevent it from leaking. > > If we really want to use a slice, I'd keep the size in a field of the C object > structure. The deallocation function should never fail, and thus should not > rely on the __info__ attribute that could be changed by the user. Good point, have gone with this option.
Review of attachment 148653 [details] [review]: Pretty good. ::: gi/pygi-argument.c @@ +1571,3 @@ + PyErr_Format(PyExc_ValueError, "couldn't find a wrapper for type '%s'", + py_type = _pygi_type_get_from_g_type(type); + Use Py_DECREF; py_type is never NULL at this point. ::: gi/pygi-boxed.c @@ +138,3 @@ + NULL, /* tp_getset */ + (PyTypeObject *)NULL, /* tp_base */ +}; It'd be nice to keep the comments nicely aligned ;-) @@ +165,3 @@ + ((PyGBoxed *)self)->boxed = boxed; + ((PyGBoxed *)self)->free_on_dealloc = free_on_dealloc; + self->size = -1; self->size is an unsigned number; use 0 if you really want to set something.
Created attachment 148721 [details] [review] Set a default constructor for boxed structs that don't have one
Review of attachment 148721 [details] [review]: Good. Thanks!