After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 602735 - Add a default constructor for boxed structures
Add a default constructor for boxed structures
Status: RESOLVED FIXED
Product: pygi
Classification: Deprecated
Component: general
unspecified
Other Linux
: Normal enhancement
: 0.6
Assigned To: Tomeu Vizoso
pygi-maint
Depends on:
Blocks:
 
 
Reported: 2009-11-23 14:53 UTC by Simon van der Linden
Modified: 2009-11-30 10:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Set a default constructor for boxed structs that don't have one (13.50 KB, patch)
2009-11-23 17:14 UTC, Tomeu Vizoso
needs-work Details | Review
Set a default constructor for boxed structs that don't have one (22.65 KB, patch)
2009-11-24 12:11 UTC, Tomeu Vizoso
needs-work Details | Review
Set a default constructor for boxed structs that don't have one (14.61 KB, patch)
2009-11-27 12:23 UTC, Tomeu Vizoso
none Details | Review
Set a default constructor for boxed structs that don't have one (22.65 KB, patch)
2009-11-27 12:24 UTC, Tomeu Vizoso
needs-work Details | Review
Set a default constructor for boxed structs that don't have one (22.94 KB, patch)
2009-11-27 13:02 UTC, Tomeu Vizoso
needs-work Details | Review
Set a default constructor for boxed structs that don't have one (22.99 KB, patch)
2009-11-28 11:10 UTC, Tomeu Vizoso
needs-work Details | Review
Set a default constructor for boxed structs that don't have one (22.58 KB, patch)
2009-11-28 16:02 UTC, Tomeu Vizoso
needs-work Details | Review
Set a default constructor for boxed structs that don't have one (22.58 KB, patch)
2009-11-30 10:03 UTC, Tomeu Vizoso
committed Details | Review

Description Simon van der Linden 2009-11-23 14:53:52 UTC
Allow boxed structures to be allocated (usually with g_new) if no default constructor is given.
Comment 1 Tomeu Vizoso 2009-11-23 17:14:08 UTC
Created attachment 148333 [details] [review]
Set a default constructor for boxed structs that don't have one
Comment 2 Simon van der Linden 2009-11-23 21:07:46 UTC
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.
Comment 3 Tomeu Vizoso 2009-11-24 12:11:32 UTC
Created attachment 148379 [details] [review]
Set a default constructor for boxed structs that don't have one
Comment 4 Simon van der Linden 2009-11-26 22:09:27 UTC
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?
Comment 5 Tomeu Vizoso 2009-11-27 12:23:45 UTC
Created attachment 148585 [details] [review]
Set a default constructor for boxed structs that don't have one
Comment 6 Tomeu Vizoso 2009-11-27 12:24:49 UTC
Created attachment 148586 [details] [review]
Set a default constructor for boxed structs that don't have one
Comment 7 Simon van der Linden 2009-11-27 12:42:41 UTC
Review of attachment 148586 [details] [review]:

Are there any differences with the patch I reviewed?!
Comment 8 Tomeu Vizoso 2009-11-27 13:02:57 UTC
Created attachment 148594 [details] [review]
Set a default constructor for boxed structs that don't have one
Comment 9 Tomeu Vizoso 2009-11-27 13:03:24 UTC
(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.
Comment 10 Simon van der Linden 2009-11-27 21:27:56 UTC
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!
Comment 11 Tomeu Vizoso 2009-11-28 11:10:08 UTC
Created attachment 148643 [details] [review]
Set a default constructor for boxed structs that don't have one
Comment 12 Simon van der Linden 2009-11-28 15:49:20 UTC
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.
Comment 13 Tomeu Vizoso 2009-11-28 16:02:16 UTC
Created attachment 148653 [details] [review]
Set a default constructor for boxed structs that don't have one
Comment 14 Tomeu Vizoso 2009-11-28 16:02:54 UTC
(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.
Comment 15 Simon van der Linden 2009-11-29 23:27:32 UTC
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.
Comment 16 Tomeu Vizoso 2009-11-30 10:03:52 UTC
Created attachment 148721 [details] [review]
Set a default constructor for boxed structs that don't have one
Comment 17 Simon van der Linden 2009-11-30 10:35:06 UTC
Review of attachment 148721 [details] [review]:

Good. Thanks!