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 547104 - improve type wrapper creation
improve type wrapper creation
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: gobject
Git master
Other All
: Normal normal
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks:
 
 
Reported: 2008-08-09 20:56 UTC by Paul Pogonyshev
Modified: 2008-08-11 20:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
improvement patch (3.11 KB, patch)
2008-08-09 20:58 UTC, Paul Pogonyshev
none Details | Review
improvement patch, with nasty pointer arithmetics hidden in a macro (3.15 KB, patch)
2008-08-10 14:46 UTC, Paul Pogonyshev
needs-work Details | Review
improvement patch, revision 3 (3.38 KB, patch)
2008-08-10 15:17 UTC, Paul Pogonyshev
none Details | Review
improvement patch, revision 4 (3.32 KB, patch)
2008-08-10 15:29 UTC, Paul Pogonyshev
committed Details | Review
improvement patch, next generation (basing on what is already committed) (6.04 KB, patch)
2008-08-10 18:58 UTC, Paul Pogonyshev
committed Details | Review
patch adding two slots to gio.AppInfo (2.19 KB, patch)
2008-08-10 19:33 UTC, Paul Pogonyshev
committed Details | Review

Description Paul Pogonyshev 2008-08-09 20:56:46 UTC
As I noted in bug #546120, we need to improve runtime type wrapper creation in order for patch in that bug to work.  Patch follows.
Comment 1 Paul Pogonyshev 2008-08-09 20:58:07 UTC
Created attachment 116264 [details] [review]
improvement patch

Patch that makes newly created type wrappers inherit some slots following some algorithm.  It is best described in comments in the patch.
Comment 2 Paul Pogonyshev 2008-08-10 14:46:03 UTC
Created attachment 116290 [details] [review]
improvement patch, with nasty pointer arithmetics hidden in a macro

Other than the macro stuff, the patch is the same.

Result of this patch + GFile slots in bug #546120:

>>> import gio
>>> f = gio.File ('test'); g = gio.File ('test')
>>> f
<__main__.GLocalFile at 0x407d570c: file:///home/paul/svn/pygobject/test>
>>> f == g
True
>>> hash(f) == hash(g)
True
Comment 3 Johan (not receiving bugmail) Dahlin 2008-08-10 14:54:36 UTC
Comment on attachment 116290 [details] [review]
improvement patch, with nasty pointer arithmetics hidden in a macro

>Index: gobject/pygobject.c

How can we test this modification?>+static void
>+pygobject_pick_slot_value(PyTypeObject *type, PyObject *bases, int slot_offset)
>+{
>+#define TYPE_SLOT_VALUE(type)				\
>+    (* (void **) (((char *) (type)) + slot_offset))
>+
>+    void *gobject_value = TYPE_SLOT_VALUE(&PyGObject_Type);
>+    void *object_value = TYPE_SLOT_VALUE(&PyBaseObject_Type);

These can probably go, let's trust the compiler to optimize for us.

>+    void *value = NULL;
>+    int k;

Why not just i?

>+    int num_bases;
>+
>+    for (k = 0, num_bases = PyTuple_Size(bases); k < num_bases; ++k) {

I'd move num_bases outside of the for.

>+	PyTypeObject *base_type = (PyTypeObject *) PyTuple_GetItem(bases, k);
>+	void *slot_value = TYPE_SLOT_VALUE(base_type);
>+
>+	if (slot_value && slot_value != gobject_value && slot_value != object_value) {
>+	    if (value) {
>+		value = NULL;
>+		break;
>+	    }
>+	    else
>+		value = slot_value;
>+	}
>+    }

Can this if be rewritten some thing like:

if (slot_value == NULL)
   continue
if (slot_value == TYPE_SLOT_VALUE(&PyGObject_Type) || 
    slot_value == TYPE_SLOT_VALUE(&PyBaseObject_Type))
   continue
if (value == NULL)
   return

value = slot_value;
  
>+
>+    if (value)
>+	TYPE_SLOT_VALUE(type) = value;

And then you could remove the if here.

>+#undef TYPE_SLOT_VALUE
Comment 4 Paul Pogonyshev 2008-08-10 15:17:35 UTC
Created attachment 116292 [details] [review]
improvement patch, revision 3

(In reply to comment #3)
[Removed parts are just done.]

> How can we test this modification?

This I frankly don't know for sure.  A couple of non-direct results:
- all PyGObject and PyGTK tests keep passing;
- the patch with GFile slots starts working as intended (and it doesn't without this patch);
- PyGObject/PyGTK actually seldom use these slots anyway (as of yet), so the impact of the patch must be limited to relatively few cases.

> Why not just i?

Oh, I'm just weird with naming conventions.  Changed.

> Can this if be rewritten some thing like:
> [...]

Done.  I also noticed one minor bug here: if multiple bases give us the same slot value (e.g. because one already inherited custom slot from another via this way), we must treat it like one value.  Fixed in this revision.

> >+
> >+    if (value)
> >+	TYPE_SLOT_VALUE(type) = value;
> 
> And then you could remove the if here.

Actually, no, I can't.  See the comments in the patch.
Comment 5 Johan (not receiving bugmail) Dahlin 2008-08-10 15:25:15 UTC
Comment on attachment 116292 [details] [review]
improvement patch, revision 3

Okay, getting better.
value -> found_slot
base_value -> slot
TYPE_SLOT_VALUE -> TYPE_SLOT
pygobject_pick_slot_value -> pygobject_find_slot_for

Right?
Comment 6 Paul Pogonyshev 2008-08-10 15:29:46 UTC
Created attachment 116293 [details] [review]
improvement patch, revision 4

Well, OK, this was at least simple :)
Comment 7 Paul Pogonyshev 2008-08-10 15:39:55 UTC
Thanks for the quick review!

Sending        ChangeLog
Sending        gobject/pygobject.c
Transmitting file data ..
Committed revision 938.

2008-08-10  Paul Pogonyshev  <pogonyshev@gmx.net>

	Bug 547104 – improve runtime type wrapper creation

	* gobject/pygobject.c (pygobject_new_with_interfaces): Use new
	pygobject_find_slot_for() for `tp_richcompare', `tp_compare`,
	`tp_hash', `tp_iter', `tp_repr', `tp_str' and `tp_print'.
	(pygobject_find_slot_for): New static function.
Comment 8 Paul Pogonyshev 2008-08-10 18:55:06 UTC
I'm reopening the bug, as I understood that we can do the same not only for runtime-created types, but for normal ones as well, without modifying codegen in any way.

Rationale for doing this also for normal types:

* Less code duplication: example is 'gicon.override' which currently has to duplicate 'tp_richcompare' and 'tp_hash' for all the different types.  In addition to code duplication being bad as it is, this is also error-prone if GLib adds more implementation, but we forget to copy custom slots for the wrappers of these new implementation.

* Sometimes we need to duplicate code across module boundaries.  E.g. types gio.AppInfo and gio.unix.DesktopAppInfo are an interface and its implementation residing in different modules.  They have a natural 'tp_richcompare' implementation, but there are technical difficulties with PyGAppInfo_Type not being available in gio.unix module.  The difficulties can be solved in other ways too, but with proposed changes we don't need to have them at all.

Patch in a minute.
Comment 9 Paul Pogonyshev 2008-08-10 18:58:08 UTC
Created attachment 116300 [details] [review]
improvement patch, next generation (basing on what is already committed)

This patch improves type creation for normal types (passed through pygobject_register_class), not only runtime-created types.  I made some restructuring of the code, but the changes are practically technical and don't make any real differences.

Additionally, I removed all duplicated in 'gicon.override' to show that it does indeed work as intended.  After removal all tests keep passing.  That wouldn't be the case without changes in 'pygobject.c'.
Comment 10 Paul Pogonyshev 2008-08-10 19:33:46 UTC
Created attachment 116302 [details] [review]
patch adding two slots to gio.AppInfo

This patch is only somewhat related to the bug in question.  However, it underlines the point of second type creation improvement patch.  It changes only slots of gio.AppInfo.  If patch in attachment 116300 [details] [review] is not applied, it makes no visible changes, because in reality all appinfos are of type gio.unix.DesktopAppInfo (implementation of gio.AppInfo).  However, when that patch is applied, the two slots from gio.AppInfo get propagated into gio.unix.DesktopAppInfo, now making a difference:

>>> gio.app_info_get_all()[0]
<gio.unix.DesktopAppInfo at 0x401fffa4: kazehakase>

Also, with both patches applied, the test_eq starts passing.
Comment 11 Paul Pogonyshev 2008-08-11 20:18:35 UTC
Approved by Johan on IRC.

Sending        ChangeLog
Sending        gio/gicon.override
Sending        gobject/pygobject.c
Transmitting file data ...
Committed revision 940.

2008-08-11  Paul Pogonyshev  <pogonyshev@gmx.net>

	Bug 547104 – improve type wrapper creation

	* gobject/pygobject.c (pygobject_register_class): Use new
	pygobject_inherit_slots() to propagate custom slots in normal
	types too.
	(pygobject_inherit_slots): New function, break out of
	pygobject_new_with_interfaces() and rewrite a bit.
	(pygobject_find_slot_for): Add new argument that can forbid
	overriding non-NULL slots.

	* gio/gicon.override (pygio_do_icon_richcompare): Remove, the code
	is now directly in _wrap_g_icon_tp_richcompare().
	(_wrap_g_file_icon_tp_richcompare, _wrap_g_file_icon_tp_hash)
	(_wrap_g_themed_icon_tp_richcompare, _wrap_g_themed_icon_tp_hash):
	Remove, duplicating code in this way is not needed anymore.

Sending        ChangeLog
Sending        gio/gappinfo.override
Sending        tests/test_gio.py
Transmitting file data ...
Committed revision 941.

2008-08-11  Paul Pogonyshev  <pogonyshev@gmx.net>

	Bug 547104 – improve type wrapper creation

	* gio/gappinfo.override (_wrap_g_app_info_tp_richcompare)
	(_wrap_g_app_info_tp_repr): New functions.

	* tests/test_gio.py (TestAppInfo.test_eq): New test.