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.

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.

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.

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.

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 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 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 on attachment 236353 [details] [review]
Unify Python callable to GClosure GI marshaling code

Thanks for this cleanup work!

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

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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 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 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.

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?

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)

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()?

Review of attachment 240040 [details] [review]:

Same comment about the _pygi* vs pygi* duality -- can we just use the already existing one?

Review of attachment 240045 [details] [review]:

Looks fine, but same comment about pygi* vs. _pygi*

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?

Review of attachment 240112 [details] [review]:

This looks fine to me, but I hope we have sufficient test coverage for this. :-)

(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?

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!

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.

Review of attachment 240716 [details] [review]:

Indeed, very nice. Thanks!

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

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.

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.

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.

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.

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.

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.

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.

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.

(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.

Thanks for the updates! Looking good to me now.

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

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.

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.

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?

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.

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.

(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.

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.

Created attachment 249669 [details] [review]
Move basic type marshaling out of _pygi_argument_to_object

Updated to use the GI terminology of "basic type".

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.

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.

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.

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.

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.

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

Created attachment 249958 [details] [review]
tests: Add C unittesting of basic type marshaling

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.

Created attachment 249960 [details] [review]
Add support for PyBytes with int8 and uint8 from Python marshaler

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.

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.

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.

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.

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.

Bug #688081 will need prior completion in order to unify void pointer marshaling.

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 on attachment 249955 [details] [review]
Replace to Python cached marshalers with unified basic type marshaler

Nice!

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.

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 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).

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?

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.

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.

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?

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.

(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.

(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?

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

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

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.

(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.

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).

Marking as fixed now that patches in bug 727004 have landed.