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 693405 - Unify and refactor direct call and vfunc/closure argument marshalers
Unify and refactor direct call and vfunc/closure argument marshalers
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
unspecified
Other Linux
: Normal normal
: GNOME 3.10
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on: 640812 688081 709700 727004
Blocks:
 
 
Reported: 2013-02-08 09:38 UTC by Simon Feltman
Modified: 2014-08-08 01:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Unify Python object to GValue GI marshaling code (7.46 KB, patch)
2013-02-16 05:03 UTC, Simon Feltman
none Details | Review
Unify Python callable to GClosure GI marshaling code (5.26 KB, patch)
2013-02-16 07:00 UTC, Simon Feltman
none Details | Review
Rename pygi_marshal_from_py_object to make it more explicit (4.70 KB, patch)
2013-02-16 07:22 UTC, Simon Feltman
committed Details | Review
Unify Python object to GValue GI marshaling code (7.45 KB, patch)
2013-02-16 07:22 UTC, Simon Feltman
committed Details | Review
Unify Python callable to GClosure GI marshaling code (4.13 KB, patch)
2013-02-16 07:22 UTC, Simon Feltman
committed Details | Review
Unify Python unicode to unichar GI marshaling code (5.55 KB, patch)
2013-03-28 11:35 UTC, Simon Feltman
accepted-commit_now Details | Review
Unify Python unicode to utf8 GI marshaling code (4.49 KB, patch)
2013-03-28 11:35 UTC, Simon Feltman
accepted-commit_now Details | Review
Unify Python unicode to filename GI marshaling code (4.86 KB, patch)
2013-03-28 11:35 UTC, Simon Feltman
accepted-commit_now Details | Review
Unify unichar to Python GI marshaling code (4.05 KB, patch)
2013-03-28 12:30 UTC, Simon Feltman
reviewed Details | Review
Unify utf8 to Python GI marshaling code (2.47 KB, patch)
2013-03-28 12:30 UTC, Simon Feltman
reviewed Details | Review
Unify filename to Python GI marshaling code (3.56 KB, patch)
2013-03-28 12:30 UTC, Simon Feltman
reviewed Details | Review
Unify Python float and double to GI marshaling code (5.42 KB, patch)
2013-03-28 13:18 UTC, Simon Feltman
reviewed Details | Review
Unify interface struct to Python GI marshaling code (16.62 KB, patch)
2013-03-29 05:44 UTC, Simon Feltman
none Details | Review
Unify interface struct to Python GI marshaling code (11.99 KB, patch)
2013-03-29 12:43 UTC, Simon Feltman
none Details | Review
Unify Python interface struct to GI marshaling code (16.62 KB, patch)
2013-03-29 12:44 UTC, Simon Feltman
reviewed Details | Review
Unify interface struct to Python GI marshaling code (11.99 KB, patch)
2013-03-29 12:45 UTC, Simon Feltman
accepted-commit_now Details | Review
Unify Python unicode to unichar GI marshaling code (2.82 KB, patch)
2013-04-05 11:07 UTC, Simon Feltman
committed Details | Review
Unify Python unicode to utf8 GI marshaling code (1.85 KB, patch)
2013-04-05 12:31 UTC, Simon Feltman
committed Details | Review
Unify Python unicode to filename GI marshaling code (1.78 KB, patch)
2013-04-05 12:31 UTC, Simon Feltman
committed Details | Review
Unify unichar to Python GI marshaling code (1.71 KB, patch)
2013-04-05 12:31 UTC, Simon Feltman
committed Details | Review
Unify utf8 to Python GI marshaling code (1.70 KB, patch)
2013-04-05 12:31 UTC, Simon Feltman
committed Details | Review
Unify filename to Python GI marshaling code (2.25 KB, patch)
2013-04-05 12:31 UTC, Simon Feltman
committed Details | Review
Unify Python float and double to GI marshaling code (4.40 KB, patch)
2013-04-05 12:31 UTC, Simon Feltman
committed Details | Review
Unify Python interface struct to GI marshaling code (16.69 KB, patch)
2013-04-05 12:31 UTC, Simon Feltman
committed Details | Review
Unify interface struct to Python GI marshaling code (11.94 KB, patch)
2013-04-05 12:31 UTC, Simon Feltman
committed Details | Review
Unify flags and enum to Python marshaling code (9.92 KB, patch)
2013-04-07 06:24 UTC, Simon Feltman
needs-work Details | Review
Unify Python flags and enums to GI marshaling code (11.76 KB, patch)
2013-04-07 11:26 UTC, Simon Feltman
needs-work Details | Review
Move simple type marshaling out of _pygi_argument_to_object (4.42 KB, patch)
2013-07-20 01:18 UTC, Simon Feltman
none Details | Review
Move basic type marshaling out of _pygi_argument_to_object (4.41 KB, patch)
2013-07-20 02:05 UTC, Simon Feltman
none Details | Review
Replace cached arg marshalers for basic types with unified marshaler (24.69 KB, patch)
2013-07-20 03:42 UTC, Simon Feltman
none Details | Review
Move basic type marshaling out of _pygi_argument_from_object (6.20 KB, patch)
2013-07-20 06:44 UTC, Simon Feltman
none Details | Review
Move to Python basic type marshaling out of _pygi_argument_to_object (4.42 KB, patch)
2013-07-23 21:30 UTC, Simon Feltman
committed Details | Review
Replace to Python cached marshalers with unified basic type marshaler (24.69 KB, patch)
2013-07-23 21:31 UTC, Simon Feltman
committed Details | Review
Move basic type marshaling out of _pygi_argument_from_object (6.20 KB, patch)
2013-07-23 21:31 UTC, Simon Feltman
committed Details | Review
Move _pygi_argument_from_object_basic_type into pygi-marshal-from-py.c (11.59 KB, patch)
2013-07-23 21:31 UTC, Simon Feltman
committed Details | Review
tests: Add C unittesting of basic type marshaling (11.29 KB, patch)
2013-07-23 21:31 UTC, Simon Feltman
needs-work Details | Review
Move from Python integer marshaling into separate function (7.85 KB, patch)
2013-07-23 21:31 UTC, Simon Feltman
committed Details | Review
Add support for PyBytes with int8 and uint8 from Python marshaler (3.31 KB, patch)
2013-07-23 21:31 UTC, Simon Feltman
committed Details | Review
Unify from Python boolean, int8, and uint8 marshalers (15.56 KB, patch)
2013-07-23 21:31 UTC, Simon Feltman
committed Details | Review
Unify from Python int16 and int32 marshalers (19.98 KB, patch)
2013-07-23 21:31 UTC, Simon Feltman
committed Details | Review
Unify from Python int64 and uint64 marshalers (12.83 KB, patch)
2013-07-23 21:31 UTC, Simon Feltman
committed Details | Review
Unify and clean up from Python marshalers for basic types (13.82 KB, patch)
2013-07-23 21:31 UTC, Simon Feltman
committed Details | Review
Move _pygi_argument_to_object_basic_type into pygi-marshal-to-py.c (11.51 KB, patch)
2013-07-23 21:31 UTC, Simon Feltman
committed Details | Review

Description Simon Feltman 2013-02-08 09:38:24 UTC
Different code paths are taken for marshaling values for vfuncs/closures and direct calls to functions/methods. While the outer call structures to complete these tasks are very different, the inner marshaling bits should be unified. This will help keep bugs fixed for both cases and allow us to break down the large switch statements for vfunc/closure marshaling into manageable pieces. 

To/from marshalers for vfunc/closures in large switch statements:
gi/pygi-argument.c

To/from marshalers for direct calls wired into call caches:
gi/pygi-marshal-to-py.c
gi/pygi-marshal-from-py.c

Some of this work as already been started in the following ticket. Further work should use this as a template for moving the code around:
https://bugzilla.gnome.org/show_bug.cgi?id=687522
Comment 1 Simon Feltman 2013-02-16 05:03:40 UTC
Created attachment 236346 [details] [review]
Unify Python object to GValue GI marshaling code

Add pygi_marshal_from_py_g_value which can be used for direct gi method
call args and vfunc out args. The new method also adds an "is_allocated"
parameter which will be used to fix future leaks.
Comment 2 Simon Feltman 2013-02-16 07:00:25 UTC
Created attachment 236350 [details] [review]
Unify Python callable to GClosure GI marshaling code

Add pygi_marshal_from_py_gclosure which can be used for direct gi method
call args and vfunc out args.
Comment 3 Simon Feltman 2013-02-16 07:22:39 UTC
Created attachment 236351 [details] [review]
Rename pygi_marshal_from_py_object to make it more explicit

Rename pygi_marshal_from_py_object to pygi_marshal_from_py_gobject
to make it more explicit and give consistency with future refactoring.
Comment 4 Simon Feltman 2013-02-16 07:22:42 UTC
Created attachment 236352 [details] [review]
Unify Python object to GValue GI marshaling code

Add pygi_marshal_from_py_g_value which can be used for direct gi method
call args and vfunc out args. The new method also adds an "is_allocated"
parameter which will be used to fix future leaks.
Comment 5 Simon Feltman 2013-02-16 07:22:43 UTC
Created attachment 236353 [details] [review]
Unify Python callable to GClosure GI marshaling code

Add pygi_marshal_from_py_gclosure which can be used for direct gi method
call args and vfunc out args.
Comment 6 Martin Pitt 2013-02-16 16:26:48 UTC
Comment on attachment 236351 [details] [review]
Rename pygi_marshal_from_py_object to make it more explicit

This is harmless (no external API change nor behaviour change), more consistent, and indepentent of the further patches, please go ahead.
Comment 7 Martin Pitt 2013-02-16 16:34:58 UTC
Comment on attachment 236352 [details] [review]
Unify Python object to GValue GI marshaling code

"which will be used to fix future leaks." → I suppose this is means "will be used to fix leaks in the future"?

The actual patch looks fine to me.
Comment 8 Martin Pitt 2013-02-16 16:37:09 UTC
Comment on attachment 236353 [details] [review]
Unify Python callable to GClosure GI marshaling code

Thanks for this cleanup work!
Comment 9 Simon Feltman 2013-02-16 21:55:37 UTC
Attachment 236351 [details] pushed as 15cd7be - Rename pygi_marshal_from_py_object to make it more explicit
Attachment 236352 [details] pushed as 9e47afe - Unify Python object to GValue GI marshaling code
Attachment 236353 [details] pushed as 93c1536 - Unify Python callable to GClosure GI marshaling code
Comment 10 Simon Feltman 2013-03-28 11:35:14 UTC
Created attachment 240027 [details] [review]
Unify Python unicode to unichar GI marshaling code

Add pygi_marshal_from_py_unichar which can be used for direct gi method
call args and vfunc out args.
Comment 11 Simon Feltman 2013-03-28 11:35:17 UTC
Created attachment 240028 [details] [review]
Unify Python unicode to utf8 GI marshaling code

Add pygi_marshal_from_py_utf8 which can be used for direct gi method
call args and vfunc out args.
Comment 12 Simon Feltman 2013-03-28 11:35:20 UTC
Created attachment 240029 [details] [review]
Unify Python unicode to filename GI marshaling code

Add pygi_marshal_from_py_utf8 which can be used for direct gi method
call args and vfunc out args.
Comment 13 Simon Feltman 2013-03-28 12:30:45 UTC
Created attachment 240038 [details] [review]
Unify unichar to Python GI marshaling code

Add pygi_marshal_to_py_unichar which can be used for direct gi method
call out args and vfunc in args.
Comment 14 Simon Feltman 2013-03-28 12:30:48 UTC
Created attachment 240039 [details] [review]
Unify utf8 to Python GI marshaling code

Add pygi_marshal_to_py_utf8 which can be used for direct gi method
call out args and vfunc in args.
Comment 15 Simon Feltman 2013-03-28 12:30:50 UTC
Created attachment 240040 [details] [review]
Unify filename to Python GI marshaling code

Add pygi_marshal_to_py_filename which can be used for direct gi method
call out args and vfunc in args.
Comment 16 Simon Feltman 2013-03-28 13:18:58 UTC
Created attachment 240045 [details] [review]
Unify Python float and double to GI marshaling code

Add pygi_marshal_from_py_float/double which can be used for direct gi method
call args and vfunc out args.
Comment 17 Simon Feltman 2013-03-29 05:44:19 UTC
Created attachment 240101 [details] [review]
Unify interface struct to Python GI marshaling code

Add pygi_marshal_from_py_interface_struct used for direct gi method
call out args and vfunc in args.
Comment 18 Simon Feltman 2013-03-29 12:43:03 UTC
Created attachment 240110 [details] [review]
Unify interface struct to Python GI marshaling code

Add pygi_marshal_to_py_interface_struct used for direct gi method
call out args and vfunc in args.
Comment 19 Simon Feltman 2013-03-29 12:44:11 UTC
Created attachment 240111 [details] [review]
Unify Python interface struct to GI marshaling code

Add pygi_marshal_from_py_interface_struct used for direct gi method
call in args and vfunc out args.
Comment 20 Simon Feltman 2013-03-29 12:45:16 UTC
Created attachment 240112 [details] [review]
Unify interface struct to Python GI marshaling code

Add pygi_marshal_to_py_interface_struct used for direct gi method
call out args and vfunc in args.
Comment 21 Simon Feltman 2013-03-30 03:01:02 UTC
For reference, implementing what was described in the following comment would be a good first step towards refactoring array marshaling:
https://bugzilla.gnome.org/show_bug.cgi?id=662241#c9
Comment 22 Martin Pitt 2013-04-04 03:56:42 UTC
Comment on attachment 240027 [details] [review]
Unify Python unicode to unichar GI marshaling code

Thanks for doing these, Simon! That's a nice code cleanup.
Comment 23 Martin Pitt 2013-04-04 04:00:56 UTC
Comment on attachment 240028 [details] [review]
Unify Python unicode to utf8 GI marshaling code

The two original implementations used a slightly different logic wrt. py 2/3 and unicode/str, but the facorized one looks fine to me.
Comment 24 Martin Pitt 2013-04-04 04:05:32 UTC
Review of attachment 240029 [details] [review]:

I know that the current code doesn't support it either, but I wonder if we should support passing a byte array in Python 3 as well? This is quite realistic to happen in actual programs, and it would avoid another bytes → unicode → bytes conversion?
Comment 25 Martin Pitt 2013-04-04 04:11:13 UTC
Review of attachment 240038 [details] [review]:

> "Invalid unicode codepoint %" G_GUINT32_FORMAT,

Mixing glib string format macros with Py*_Format() macros has already lead to broken error messages more than once. Can we please use %lu and case the value to (unsigned long)? (See http://docs.python.org/2/c-api/string.html#PyString_FromFormat for allowed macros).

(I know the previous code already does that, but let's clean this up while we are at it)
Comment 26 Martin Pitt 2013-04-04 04:19:22 UTC
Review of attachment 240039 [details] [review]:

The _pygi_marshal_to_py_utf8() vs. pygi_marshal_to_py_utf8() duality is a bit confusing. Perhaps we could just call _pygi_marshal_to_py_utf8() with NULL state/cache arguments in _pygi_argument_to_object()?
Comment 27 Martin Pitt 2013-04-04 04:21:24 UTC
Review of attachment 240040 [details] [review]:

Same comment about the _pygi* vs pygi* duality -- can we just use the already existing one?
Comment 28 Martin Pitt 2013-04-04 04:24:01 UTC
Review of attachment 240045 [details] [review]:

Looks fine, but same comment about pygi* vs. _pygi*
Comment 29 Martin Pitt 2013-04-04 04:31:30 UTC
Review of attachment 240111 [details] [review]:

Here the _pygi* vs pygi_* split is justified IMHO, as they have wildly different arguments.

::: gi/pygi-argument.c
@@ +1161,3 @@
+                                                           transfer,
+                                                           FALSE, /*is_caller_allocates*/
+                                                           g_struct_info_is_foreign (info));

Doesn't py_type need to be DECREFed here? Or is this an unowned pointer?
Comment 30 Martin Pitt 2013-04-04 04:33:47 UTC
Review of attachment 240112 [details] [review]:

This looks fine to me, but I hope we have sufficient test coverage for this. :-)
Comment 31 Simon Feltman 2013-04-05 02:36:54 UTC
(In reply to comment #26)
> Review of attachment 240039 [details] [review]:
> 
> The _pygi_marshal_to_py_utf8() vs. pygi_marshal_to_py_utf8() duality is a bit
> confusing. Perhaps we could just call _pygi_marshal_to_py_utf8() with NULL
> state/cache arguments in _pygi_argument_to_object()?

I think this might be a better approach in general for any function which doesn't need additional args beyond the GIArgument or PyObject. I can go through the previous refactors and do this as well.

Another possibility for marshalers which require more information is we could build up temporary versions of the caches and fill in relevant information. Eventually I hope all of this can go away and we can just use caches for everything. In this sense everything I'm doing is somewhat temporary. But at the same time I don't want to change things to hastily in an attempt to avoid regressions. What do you think?
Comment 32 Martin Pitt 2013-04-05 07:05:41 UTC
Yes, I agree that small steps are always better, so the whole cache handling shouldn't be re-enginered (or rather, introduced) in pygi-argument.c. Hence my proposal to keep the dual functions for complex situations (such as for structs), and just use the _pygi variant with two NULL arguments for simple ones (such as for strings).

I'm not sure whether building temporary caches will actually help,  it would add extra CPU cycles without much benefit. Except if doing that is a transitional step in unifying the existing cache handling for methods with those of vfuncs/callbacks, of course.

Thanks!
Comment 33 Simon Feltman 2013-04-05 11:07:40 UTC
Created attachment 240716 [details] [review]
Unify Python unicode to unichar GI marshaling code

Change _pygi_argument_from_object to use the cachers marshaler
(_pygi_marshal_from_py_unichar) directly instead of keeping a
copy of the code.
Comment 34 Martin Pitt 2013-04-05 11:10:54 UTC
Review of attachment 240716 [details] [review]:

Indeed, very nice. Thanks!
Comment 35 Simon Feltman 2013-04-05 11:14:11 UTC
Comment on attachment 240716 [details] [review]
Unify Python unicode to unichar GI marshaling code

Attachment 240716 [details] pushed as e253c73 - Unify Python unicode to unichar GI marshaling code
Comment 36 Simon Feltman 2013-04-05 12:31:05 UTC
Created attachment 240727 [details] [review]
Unify Python unicode to utf8 GI marshaling code

Change _pygi_argument_from_object to use the cachers marshaler
(_pygi_marshal_from_py_utf8) directly instead of keeping a
copy of the code.
Comment 37 Simon Feltman 2013-04-05 12:31:08 UTC
Created attachment 240728 [details] [review]
Unify Python unicode to filename GI marshaling code

Change _pygi_argument_from_object to use the cachers marshaler
(_pygi_marshal_from_py_filename) directly instead of keeping a
copy of the code.
Comment 38 Simon Feltman 2013-04-05 12:31:11 UTC
Created attachment 240729 [details] [review]
Unify unichar to Python GI marshaling code

Change _pygi_argument_to_object to use the cachers marshaler
(_pygi_marshal_to_py_unichar) directly instead of keeping a
copy of the code.
Comment 39 Simon Feltman 2013-04-05 12:31:14 UTC
Created attachment 240730 [details] [review]
Unify utf8 to Python GI marshaling code

Change _pygi_argument_to_object to use the cachers marshaler
(_pygi_marshal_to_py_utf8) directly instead of keeping a
copy of the code.
Comment 40 Simon Feltman 2013-04-05 12:31:17 UTC
Created attachment 240731 [details] [review]
Unify filename to Python GI marshaling code

Change _pygi_argument_to_object to use the cachers marshaler
(_pygi_marshal_to_py_filename) directly instead of keeping a
copy of the code.
Comment 41 Simon Feltman 2013-04-05 12:31:20 UTC
Created attachment 240732 [details] [review]
Unify Python float and double to GI marshaling code

Change _pygi_argument_from_object to use the cachers marshalers
(_pygi_marshal_from_py_float/double) directly instead of keeping a
copy of the code.
Refactor _pygi_marshal_from_py_float/double to use a common utility
_pygi_py_arg_to_double for initial error checking and conversion.
Comment 42 Simon Feltman 2013-04-05 12:31:23 UTC
Created attachment 240733 [details] [review]
Unify Python interface struct to GI marshaling code

Add pygi_marshal_from_py_interface_struct used for direct gi method
call in args and vfunc out args.
Comment 43 Simon Feltman 2013-04-05 12:31:27 UTC
Created attachment 240734 [details] [review]
Unify interface struct to Python GI marshaling code

Add pygi_marshal_to_py_interface_struct used for direct gi method
call out args and vfunc in args.
Comment 44 Simon Feltman 2013-04-05 12:35:24 UTC
(In reply to comment #29)
> Review of attachment 240111 [details] [review]:
> 
> Here the _pygi* vs pygi_* split is justified IMHO, as they have wildly
> different arguments.
> 
> ::: gi/pygi-argument.c
> @@ +1161,3 @@
> +                                                           transfer,
> +                                                           FALSE,
> /*is_caller_allocates*/
> +                                                          
> g_struct_info_is_foreign (info));
> 
> Doesn't py_type need to be DECREFed here? Or is this an unowned pointer?

That did need a DECREF, nice catch! Fixed in new patch.
Comment 45 Martin Pitt 2013-04-05 12:43:55 UTC
Thanks for the updates! Looking good to me now.
Comment 46 Simon Feltman 2013-04-05 12:54:25 UTC
Attachment 240727 [details] pushed as a62e8cd - Unify Python unicode to utf8 GI marshaling code
Attachment 240728 [details] pushed as 594ad08 - Unify Python unicode to filename GI marshaling code
Attachment 240729 [details] pushed as 03ff41a - Unify unichar to Python GI marshaling code
Attachment 240730 [details] pushed as 54aa043 - Unify utf8 to Python GI marshaling code
Attachment 240731 [details] pushed as 2eb2a71 - Unify filename to Python GI marshaling code
Attachment 240732 [details] pushed as 1ea654b - Unify Python float and double to GI marshaling code
Attachment 240733 [details] pushed as 6d3a075 - Unify Python interface struct to GI marshaling code
Attachment 240734 [details] pushed as 09610bf - Unify interface struct to Python GI marshaling code
Comment 47 Simon Feltman 2013-04-07 06:24:21 UTC
Created attachment 240871 [details] [review]
Unify flags and enum to Python marshaling code

Add pygi_marshal_to_py_interface_enum used for direct gi method
call out args and vfunc in args.
Change cached marshaler for flags to use the enum version because
they are so similar.
Remove dead code which was building a Python arg list that was
not being used.
Comment 48 Simon Feltman 2013-04-07 11:26:00 UTC
Created attachment 240878 [details] [review]
Unify Python flags and enums to GI marshaling code

Add pygi_marshal_from_py_interface_enum used for direct gi method
call in args and vfunc out args.
Unify flags and enum marshaling into a single method.
Comment 49 Martin Pitt 2013-04-18 05:03:37 UTC
Review of attachment 240871 [details] [review]:

Treating enums and flags uniformly has bitten us quite often in the past, so we need to make double-sure that we don't mix gint (enum) and guint (flags) anywhere. Cleaning up the duplication will certainly help here, but let's do it in a robust way. The gist is certainly in pyg_enum_from_gtype() vs. pyg_flags_from_gtype, so we need to be careful to pass along the types correctly. I'm looking forward to having this centralized, thanks for doing this!

::: gi/pygi-argument.c
@@ +1599,3 @@
+                    py_type = _pygi_type_import_by_gi_info (info);
+
+                    object = pygi_marshal_to_py_interface_enum (arg,

Calling _enum for both TYPE_ENUM and TYPE_FLAGS looks like a bug which will raise some eyebrows later on. Can we rename this to pygi_marshal_to_py_interface_enum_or_flags() and add a documentation to the function that explains that a valid info_type must always be supplied?

::: gi/pygi-cache.c
@@ +806,1 @@
                _arg_cache_from_py_interface_flags_setup (arg_cache, transfer);

Doesn't that call need to be changed as well?

@@ +808,2 @@
             if (direction == PYGI_DIRECTION_TO_PYTHON || direction == PYGI_DIRECTION_BIDIRECTIONAL)
+               _arg_cache_to_py_interface_enum_setup (arg_cache, transfer);

Likewise, if we are going to use this for both enums and flags, can we rename it to ..._enum_flags_setup()?

::: gi/pygi-marshal-to-py.c
@@ +888,3 @@
+                                   GITypeInfo *type_info,
+                                   GType g_type,
+                                   PyObject *py_type)

So the assumption here is that we always need a valid info_type and type_info, but the g_type is optional, right?

@@ +898,3 @@
+
+    if (!gi_argument_to_c_long (arg, &c_long,
+                                g_enum_info_get_storage_type ((GIEnumInfo *)interface))) {

This looks wrong when calling it for flags.

@@ +903,3 @@
+
+    if (g_type == G_TYPE_NONE) {
+        py_obj = PyObject_CallFunction (py_type, "l", c_long);

Likewise, we don't know whether we are dealing with flags here, in which case a signed long would be wrong?

Perhaps it would be better to do the case separation on the outside on info_type, and only handle the "with/without gtype" cases in the branch where we already asserted whether info_type == GI_INFO_TYPE_ENUM/_FLAGS?
Comment 50 Martin Pitt 2013-04-18 05:19:46 UTC
Review of attachment 240878 [details] [review]:

::: gi/pygi-argument.c
@@ +1179,3 @@
+                                                         info,
+                                                         type_info,
+                                                         py_type);

Similarly to the previous patch, can we rename this to pygi_marshal_from_py_interface_enum_flags()? But unlike the C->Python direction, this Python -> C direction is actually considerably different between flags and enums, so I wonder whether merging them into one function really simplifies things that much.

@@ -1181,3 @@
-                    }
-
-                    arg.v_int = PYGLIB_PyLong_AsLong (int_);

This is definitively wrong in the current code. For flags this needs to be unsigned, so good that we eliminiate this now and can worry about it in only one place.

::: gi/pygi-cache.c
@@ +790,3 @@
         case GI_INFO_TYPE_FLAGS:
             if (direction == PYGI_DIRECTION_FROM_PYTHON || direction == PYGI_DIRECTION_BIDIRECTIONAL)
+               _arg_cache_from_py_interface_enum_setup (arg_cache, transfer);

Likewise, _enum_flags_setup() would be better here.

::: gi/pygi-marshal-from-py.c
@@ +1952,3 @@
+    Py_DECREF (py_long);
+
+    interface = g_type_info_get_interface (type_info);

OOI, how is interface different from interface_info?

@@ +1959,3 @@
+    if (!gi_argument_from_c_long(arg,
+                                 c_long,
+                                 g_enum_info_get_storage_type ((GIEnumInfo *)interface))) {

This again looks wrong for flags. At this point we haven't asserted that we are dealing with an enum?

For flags this needs to use unsigned arithmetic. Also, the error handling below is completely different for flags vs. enums. This is where I wonder whether it wouldn't be better to keep two separate functions for flags and enums, as the common code is rather small.
Comment 51 Simon Feltman 2013-04-18 22:47:47 UTC
Thanks for reviewing. The intention of the patches was to just unify the code paths, not necessarily fix existing issues. But I do share your concerns, so at this point it might make sense to do more ground work refactoring with basic types (int, uint, etc...) before the enum/flags refactoring. This might allow for better re-use of those portions of the code.

The idea is to remove all the individual integer related cachers (_pygi_marshal_to_py_uint8/int8/uint16/etc...) and move them into a single cacher which uses a sub-set of the switch statement found in _pygi_argument_to_object. I don't think a simplified switch handling only int types will have much of a performance penalty in this case (need to test of course). This generalized int marshaler could then be used with the results of g_enum_info_get_storage_type to do the correct thing on all systems for enums and flags.

How does that sound?

(In reply to comment #49)
> @@ +898,3 @@
> +
> +    if (!gi_argument_to_c_long (arg, &c_long,
> +                                g_enum_info_get_storage_type ((GIEnumInfo
> *)interface))) {
> 
> This looks wrong when calling it for flags.

This looked like a bug in the original code to me as well. However, there is no GIFlagsInfo or g_flags_info_get_storage_type in GI, so perhaps a comment is what is needed. (this is a good example of why we should be using naming like "_enum_or_flags" as you described).

> @@ +903,3 @@
> +
> +    if (g_type == G_TYPE_NONE) {
> +        py_obj = PyObject_CallFunction (py_type, "l", c_long);
> 
> Likewise, we don't know whether we are dealing with flags here, in which case a
> signed long would be wrong?
> 
> Perhaps it would be better to do the case separation on the outside on
> info_type, and only handle the "with/without gtype" cases in the branch where
> we already asserted whether info_type == GI_INFO_TYPE_ENUM/_FLAGS?

I bet we can break things in the current code by marshaling a flag with the high bit set. So a test for this is probably a good idea first and foremost is.
Comment 52 Martin Pitt 2013-04-22 05:58:11 UTC
(In reply to comment #51)
> Thanks for reviewing. The intention of the patches was to just unify the code
> paths, not necessarily fix existing issues.

Right, but I wanted to note down the issues while I was looking at the code anyway. I'm okay if you want to keep the existing int/uint confusion for flags for a little longer and we file separate bugs for them. But I think this question is relevant for whether it actually makes sense to unify the enum and flags marshallers; it looks to me as if they are already rather different so that it would be more robust to keep them separate.

> The idea is to remove all the individual integer related cachers
> (_pygi_marshal_to_py_uint8/int8/uint16/etc...) and move them into a single
> cacher which uses a sub-set of the switch statement found in
> _pygi_argument_to_object. I don't think a simplified switch handling only int
> types will have much of a performance penalty in this case (need to test of
> course). This generalized int marshaler could then be used with the results of
> g_enum_info_get_storage_type to do the correct thing on all systems for enums
> and flags.
> 
> How does that sound?

If merging all the individual int cachers removes a lot of redundant code, unifying them sounds nice. It would certainly ease the code for marshalling enums indeed. Right now we really only support "int" enums AFAICS, and I yet have to come across some code which uses a different type for enums; g-i's marshalling tests have some unsigned enums AFAIR, and we have disabled tests for these which fail. So those might actually start to work when this is done.

> (In reply to comment #49)
> > @@ +898,3 @@
> > +
> > +    if (!gi_argument_to_c_long (arg, &c_long,
> > +                                g_enum_info_get_storage_type ((GIEnumInfo
> > *)interface))) {
> > 
> > This looks wrong when calling it for flags.
> 
> This looked like a bug in the original code to me as well. However, there is no
> GIFlagsInfo or g_flags_info_get_storage_type in GI, so perhaps a comment is
> what is needed.

I don't think it's missing. GFlags _always_ seems to be guint, so we just need to find out how many bits a guint has on that platform.
Comment 53 Simon Feltman 2013-07-20 01:18:22 UTC
Created attachment 249668 [details] [review]
Move simple type marshaling out of _pygi_argument_to_object

Move the marshaling of GI arguments to Python objects for simple types
into a new function. The basic required information for this marshaler
is a GITypeTag and GITransfer. Argument marshaling matching these
requirments are now found in: _pygi_argument_to_object_simple_type.
The new marshaler can be used with a generic argument cache marshaler
to unify all of the "simple" type marshaling.
Comment 54 Simon Feltman 2013-07-20 02:05:52 UTC
Created attachment 249669 [details] [review]
Move basic type marshaling out of _pygi_argument_to_object

Updated to use the GI terminology of "basic type".
Comment 55 Simon Feltman 2013-07-20 03:42:00 UTC
Created attachment 249672 [details] [review]
Replace cached arg marshalers for basic types with unified marshaler

Add cached arg marshaler "_pygi_marshal_to_py_basic_type" which
unifies functions, vfuncs, signals, and property marshaling for
"basic types". Remove all the individual cached arg marshalers
for these types.

Notes:
This essentially adds an extra function call and switch statement to
basic type marshaling for GI function call arguments. I could not
confirm or deny any noticeable performance implications of this change.
Performance tests varied so widely between rebuilds of PyGObject
with and without the change, that it basically seems to be a wash.
For example, testing GLib.get_real_time() would vary anywhere
from 4.09us to 4.34us with or without the change. Best times I've
picked up so far:

Before:
%timeit GLib.get_real_time()
100000 loops, best of 3: 4.09 us per loop

%timeit GLib.get_application_name()
100000 loops, best of 3: 4.43 us per loop

After:
%timeit GLib.get_real_time()
100000 loops, best of 3: 4.11 us per loop

%timeit GLib.get_application_name()
100000 loops, best of 3: 4.18 us per loop

I compiled with CFLAGS="-O2" for PyGObject only, GLib was using -O0
during the performance testing.
Comment 56 Simon Feltman 2013-07-20 06:44:02 UTC
Created attachment 249675 [details] [review]
Move basic type marshaling out of _pygi_argument_from_object

Move the marshaling of Python objects to GI arguments for basic types
into a new function: _pygi_argument_from_object_basic_type
This is staging work needed before unifying basic type marshaling
of arguments from Python to GI.
Comment 57 Simon Feltman 2013-07-23 21:30:58 UTC
Created attachment 249954 [details] [review]
Move to Python basic type marshaling out of _pygi_argument_to_object

Move the marshaling of GI arguments to Python objects for basic types
into a new function. The required information for this marshaler
is a GITypeTag and GITransfer. Argument marshaling matching these
requirments are now found in: _pygi_argument_to_object_basic_type.
The new marshaler can be used with a generic argument cache marshaler
to unify all of the "basic type" marshaling.
Comment 58 Simon Feltman 2013-07-23 21:31:03 UTC
Created attachment 249955 [details] [review]
Replace to Python cached marshalers with unified basic type marshaler

Add cached arg marshaler "_pygi_marshal_to_py_basic_type" which
unifies functions, vfuncs, signals, and property marshaling for
"basic types". Remove all the individual cached arg marshalers
for these types.
Comment 59 Simon Feltman 2013-07-23 21:31:07 UTC
Created attachment 249956 [details] [review]
Move basic type marshaling out of _pygi_argument_from_object

Move the marshaling of Python objects to GI arguments for basic types
into a new function: _pygi_argument_from_object_basic_type
This is staging work needed before unifying basic type marshaling
of arguments from Python to GI.
Comment 60 Simon Feltman 2013-07-23 21:31:10 UTC
Created attachment 249957 [details] [review]
Move _pygi_argument_from_object_basic_type into pygi-marshal-from-py.c

Move _pygi_argument_from_object_basic_type into pygi-marshal-from-py.c
and rename to: _pygi_marshal_from_py_basic_type
Comment 61 Simon Feltman 2013-07-23 21:31:13 UTC
Created attachment 249958 [details] [review]
tests: Add C unittesting of basic type marshaling
Comment 62 Simon Feltman 2013-07-23 21:31:17 UTC
Created attachment 249959 [details] [review]
Move from Python integer marshaling into separate function

Add _pygi_marshal_from_py_long for marshaling Python objects
that can convert to a PyLong type. This allows for better
sharing of code amongst marshalers along with unifying
them across Python 2.7 and 3.0.
Comment 63 Simon Feltman 2013-07-23 21:31:20 UTC
Created attachment 249960 [details] [review]
Add support for PyBytes with int8 and uint8 from Python marshaler
Comment 64 Simon Feltman 2013-07-23 21:31:23 UTC
Created attachment 249961 [details] [review]
Unify from Python boolean, int8, and uint8 marshalers

Replaced boolean, int8, and uint8 cached marshalers with usage of
unified basic type marshaler. Add bounds checking to unified int8
marshalers.
Comment 65 Simon Feltman 2013-07-23 21:31:27 UTC
Created attachment 249962 [details] [review]
Unify from Python int16 and int32 marshalers

Add PyNumber_Check to unified basic type marshaler.
Add bounds checking to unified int16 and int32 marshalers.
Replaced int16 and int32 cached marshalers with usage of
unified basic type marshaler.
Comment 66 Simon Feltman 2013-07-23 21:31:30 UTC
Created attachment 249963 [details] [review]
Unify from Python int64 and uint64 marshalers

Replaced int64 and uint64 cached marshalers with usage of the
unified basic type marshaler. Replace a large amount of int64
exception formatting code with usage of %S for Python 3 and
give a more vague message for Python 2.
Comment 67 Simon Feltman 2013-07-23 21:31:34 UTC
Created attachment 249964 [details] [review]
Unify and clean up from Python marshalers for basic types

Unify and cleanup boolean, float, double, gtype, unichar, utf8,
and filename marshalers.
Comment 68 Simon Feltman 2013-07-23 21:31:37 UTC
Created attachment 249965 [details] [review]
Move _pygi_argument_to_object_basic_type into pygi-marshal-to-py.c

Move _pygi_argument_to_object_basic_type into pygi-marshal-to-py.c
and rename to __pygi_marshal_to_py_basic_type.
Cleanup and simplify dependant sub-marshalers for unichar, utf8,
and filename types.
Comment 69 Simon Feltman 2013-07-24 06:20:51 UTC
Bug #688081 will need prior completion in order to unify void pointer marshaling.
Comment 70 Martin Pitt 2013-07-25 12:30:35 UTC
Comment on attachment 249954 [details] [review]
Move to Python basic type marshaling out of _pygi_argument_to_object

This looks simple enough and correct.
Comment 71 Martin Pitt 2013-07-25 12:33:35 UTC
Comment on attachment 249955 [details] [review]
Replace to Python cached marshalers with unified basic type marshaler

Nice!
Comment 72 Martin Pitt 2013-07-25 12:36:28 UTC
Comment on attachment 249957 [details] [review]
Move _pygi_argument_from_object_basic_type into pygi-marshal-from-py.c

I think this would be nicer if it was merged into the previous patch, but ok.
Comment 73 Martin Pitt 2013-07-25 12:43:16 UTC
Review of attachment 249958 [details] [review]:

Looks ok otherwise, thank you!

::: gi/Makefile.am
@@ +91,1 @@
 

I think this hunk belongs into the previous patch (https://bugzilla.gnome.org/attachment.cgi?id=249957)?

::: tests/Makefile.am
@@ +91,1 @@
 

Please fix the indentation here. Are you perhaps using somethign where a tab is not 8 spaces? :-)
Comment 74 Martin Pitt 2013-07-25 12:49:01 UTC
Comment on attachment 249960 [details] [review]
Add support for PyBytes with int8 and uint8 from Python marshaler

I don't understand the purpose of this: Why would something like b'a' or [42] be a valid representation of an integer/int8/uint8 argument? That seems confusing to me and might potentially hide programming errors (trying to pass a string/array as an int argument which only works sometimes, i. e. when the list lenght is 1).
Comment 75 Martin Pitt 2013-07-25 12:53:10 UTC
Review of attachment 249961 [details] [review]:

Otherwise looks good to me, thanks!

::: gi/pygi-marshal-from-py.h
@@ +182,1 @@
 

It is confusing to have these two names which almost look the same; could we perhaps drop one and put the "adapter" argument reshuffling into the few places where the other is actually being used?
Comment 76 Martin Pitt 2013-07-25 12:58:08 UTC
Review of attachment 249963 [details] [review]:

Looks good otherwise, please push with the typo fix.

::: tests/test_marshaling_basic_types.c
@@ +159,3 @@
+    TEST_INT_STR_ERROR (v_int64,
+                        GI_TYPE_TAG_INT64,
+                        "-0x8000000000000001", /* LL_MAXINT64-1 or -(2^63+1) */

That's LL_MININT64.
Comment 77 Martin Pitt 2013-07-25 12:58:09 UTC
Review of attachment 249963 [details] [review]:

Looks good otherwise, please push with the typo fix.

::: tests/test_marshaling_basic_types.c
@@ +159,3 @@
+    TEST_INT_STR_ERROR (v_int64,
+                        GI_TYPE_TAG_INT64,
+                        "-0x8000000000000001", /* LL_MAXINT64-1 or -(2^63+1) */

That's LL_MININT64.
Comment 78 Martin Pitt 2013-07-25 13:02:41 UTC
Review of attachment 249965 [details] [review]:

LGTM otherwise.

Many thanks for this fine refactoring!

::: gi/pygi-marshal-to-py.h
@@ +28,3 @@
 PyObject *_pygi_marshal_to_py_basic_type (PyGIInvokeState   *state,
                                           PyGICallableCache *callable_cache,
                                           PyGIArgCache      *arg_cache,

Similarly to https://bugzilla.gnome.org/attachment.cgi?id=249961, is ita actually required to have these two, or could we drop one and put the adaption of arguments into the callers? If we need both (as function pointer signatures require both), could we rename them to something explicit?
Comment 79 Simon Feltman 2013-07-25 22:21:42 UTC
Review of attachment 249958 [details] [review]:

::: gi/Makefile.am
@@ +91,1 @@
 

The only reason the symbol export is needed is specifically for this added test, so it seems like this patch was the right place for it.

::: tests/Makefile.am
@@ +91,1 @@
 

Indeed! I will fix that up.
Comment 80 Simon Feltman 2013-07-25 22:40:30 UTC
(In reply to comment #74)
> (From update of attachment 249960 [details] [review])
> I don't understand the purpose of this: Why would something like b'a' or [42]
> be a valid representation of an integer/int8/uint8 argument? That seems
> confusing to me and might potentially hide programming errors (trying to pass a
> string/array as an int argument which only works sometimes, i. e. when the list
> lenght is 1).

This only marshals single item PyBytes/PyStrings, not a list, unless I'm missing something?

I'm not clear on the exact details of why this is needed but it is to match cached marshaler which already did this:
https://git.gnome.org/browse/pygobject/tree/gi/pygi-marshal-from-py.c?id=3.9.2#n276
https://git.gnome.org/browse/pygobject/commit/?id=a558d3d3

I definitely skimped out on the commit message with this one. Will add some description that it is needed in order to unify marshalers without regressing.
Comment 81 Simon Feltman 2013-07-25 22:50:34 UTC
(In reply to comment #75)
> Review of attachment 249961 [details] [review]:
> 
> Otherwise looks good to me, thanks!
> 
> ::: gi/pygi-marshal-from-py.h
> @@ +182,1 @@
> 
> 
> It is confusing to have these two names which almost look the same; could we
> perhaps drop one and put the "adapter" argument reshuffling into the few places
> where the other is actually being used?

I agree it is confusing but I think we still need two functions. The one which takes cache arguments is needed because its function pointer stored generically for the cached marshaling. The more basic version is needed for calls which don't have a cache. Perhaps naming the one which uses caches to something like:

_pygi_marshal_from_py_basic_type_cache_adapter?
Comment 82 Simon Feltman 2013-07-25 23:30:20 UTC
Attachment 249954 [details] pushed as 663fe58 - Move to Python basic type marshaling out of _pygi_argument_to_object
Attachment 249955 [details] pushed as 0e24415 - Replace to Python cached marshalers with unified basic type marshaler
Attachment 249956 [details] pushed as 9c9510e - Move basic type marshaling out of _pygi_argument_from_object
Attachment 249957 [details] pushed as f7748af - Move _pygi_argument_from_object_basic_type into pygi-marshal-from-py.c
Comment 83 Simon Feltman 2013-07-26 00:37:47 UTC
Attachment 249959 [details] pushed as fe9df90 - Move from Python integer marshaling into separate function
Attachment 249960 [details] pushed as f517bfb - Add support for PyBytes with int8 and uint8 from Python marshaler
Attachment 249961 [details] pushed as 4b9c725 - Unify from Python boolean, int8, and uint8 marshalers
Attachment 249962 [details] pushed as 4665392 - Unify from Python int16 and int32 marshalers
Attachment 249963 [details] pushed as 9e6e01d - Unify from Python int64 and uint64 marshalers
Attachment 249964 [details] pushed as cba401a - Unify and clean up from Python marshalers for basic types
Attachment 249965 [details] pushed as fae5804 - Move _pygi_argument_to_object_basic_type into pygi-marshal-to-py.c
Comment 84 Simon Feltman 2013-07-26 00:42:45 UTC
I committed most of the patches with the exception of the new C unittests. These were having a problem PYTHON_LIBS being empty, hence the line:

# HACK because this does not seem to be setup properly by python.m4
PYTHON_LIBS=`python$(PYTHON_VERSION)-config --ldflags`

This should be fixed properly before committing, I just don't know how...

I ended up using a "_cache_adapter" suffix for the confusingly named functions. If this is agreeable then we should also change all the other functions which suffer the same confusion in the marshaler files.
Comment 85 Martin Pitt 2013-07-26 04:03:17 UTC
(In reply to comment #84)
> I ended up using a "_cache_adapter" suffix for the confusingly named functions.

Thanks, this sounds good to me, too.
Comment 86 Simon Feltman 2013-10-11 04:34:10 UTC
Marking as depending up bug 709700. The remaining marshaling unification todo is: GPtrArray, GArray, GList, GSList, GHashTable, GEnum, and GFlags. A lot could be done for flags and enums without bug 709700. But the rest is probably too complex to attempt sharing code due to needing sub-marhalers. In this respect cleaning up the architecture and using the cacher system for all marshaling code paths is the way to go. Any none GI accessible code path can form their own version of caches using what information they have (and then cache this information for later use) and then follow by using the caching marshalers. We would also need to ensure no GI based APIs are called within cached marshaler code paths. This just means we probably need to make marshalers a bit more granular by doing any GI based dispatching in cache creation (which is faster anyway).
Comment 87 Simon Feltman 2014-08-08 01:09:59 UTC
Marking as fixed now that patches in bug 727004 have landed.