Created attachment 166781 [details] [review]
Implement keyword argument handling

Created attachment 166802 [details] [review]
Implement keyword argument handling

Created attachment 166803 [details] [review]
Implement keyword argument handling

This version passes all tests and doesn't break any other tests.

Created attachment 166804 [details] [review]
Implement keyword argument handling

This is (actually) the updated version which passes tests.  Forgot to actually git add.....

Created attachment 166805 [details] [review]
Implement keyword argument handling

Same as last version but reverts an unneeded change to _free_invokation_state

removing the gio tests makes this pass the tests.  I think it might be a 64bit issue with gio itself

J5, yes it is. I had experienced it before Zach touched anything.

*** Bug 607809 has been marked as a duplicate of this bug. ***

Review of attachment 166805 [details] [review]:

A few remarks:
 - be careful with variable types and use Py_ssize_t when needed to avoid 64-bit-related issues.
 - keep variables as narrow-scoped as possible (the value variable for instance)
 - stick with the overall coding style (align arguments properly, don't add spaces to variable casts, etc.)

::: gi/pygi-invoke.c
@@ +1025,3 @@
+
+        value = NULL;
+        

Does this support multiple kw arguments correctly? Seems like arg_index need to be set to 0 here. Use a for instead of a while.

@@ +1057,2 @@
 {
+    PyObject *old_py_args = py_args;

Doesn't look useful.

@@ +1059,3 @@
     struct invocation_state state = { 0, };
 
+    py_args = _py_argument_combine(self->info, old_py_args, py_kwargs);

I think this is not the right place to put this code. It could easily be added to _prepare_invocation_state, after we fetched the arg_info and counted the number of arguments.

::: tests/test_gi.py
@@ +10,2 @@
 import gobject
+from gi.repository import GIMarshallingTests, Everything

Looks like this test need to be in test_everything.py rather than test_gi.py.

@@ +1575,3 @@
+        object_ = GIMarshallingTests.Object.new(int_=42)
+        self.assertTrue(object_)
+

I'd like you to add a few tests:
 - one with keyword arguments not in order
 - one with a keyword argument that is not listed in the function's arguments
 - one with a duplicate argument (once as list argument, once as keyword argument)

Created attachment 166820 [details] [review]
Implement keyword argument handling

This version should fix all of simon's complaints except for the moving of when the code gets called.

Review of attachment 166820 [details] [review]:

Zach, please read your patch again, and really take my comments into account. You _need_ to use the right types. And I think the logic is not exactly right.

In addition, I think the logic is not exactly right. And don't add more tests than necessary. And use self.assertRaises instead of try-except.

Thanks.

I should have read my comment twice before posting it actually :-)

Review of attachment 166820 [details] [review]:

Zach, here is a more in-depth review of your patch, as you requested.

::: gi/pygi-invoke.c
@@ +982,2 @@
 PyObject *
+_py_argument_combine(GIFunctionInfo *info,

Rename info to function_info, since you manipulate other info objects.

@@ +998,3 @@
+    int arg_index;
+
+    n_py_list_args = PyTuple_Size (py_args);

PyTuple_Size returns a Py_ssize_t.

@@ +999,3 @@
+
+    n_py_list_args = PyTuple_Size (py_args);
+    n_py_kw_args = PyMapping_Length (py_kwargs);

Ditto. Use PyDict_Size instead, since you know py_kwargs is a PyDict.

@@ +1000,3 @@
+    n_py_list_args = PyTuple_Size (py_args);
+    n_py_kw_args = PyMapping_Length (py_kwargs);
+    n_py_args = n_py_list_args + n_py_kw_args;

Must be a Py_ssize_t too. I'd like you to name it n_total_py_args, being the sum of n_py_list_args (ideally named n_py_args) and n_py_kw_args (ideally named n_py_kwargs).

@@ +1004,3 @@
+    n_args = g_callable_info_get_n_args ((GICallableInfo *) info);
+
+    new_py_args = PyTuple_New (n_py_args);

Assert that it doesn't fail.

If I pass enough keyword arguments that do not apply, I pass the count check later. Since you pass None for keyword arguments not found, None may be passed for a nullable argument, even though the default value is not None, and I didn't pass it. This is wrong.

@@ +1006,3 @@
+    new_py_args = PyTuple_New (n_py_args);
+
+    for (i = 0; i < n_py_list_args; i++) {

Declare py_arg here.

@@ +1009,3 @@
+        py_arg = PyTuple_GetItem (py_args, i);
+        Py_INCREF (py_arg);
+        PyTuple_SetItem (new_py_args, i, py_arg);

Use GET_ITEM and SET_ITEM instead of GetItem and SetItem.

@@ +1012,3 @@
+    }
+    
+    cur_tuple_index = n_py_list_args;

Rename cur_tuple_index to new_py_args_pos.

@@ +1014,3 @@
+    cur_tuple_index = n_py_list_args;
+
+    arg_index = 0;

Call it arg_pos.

@@ +1015,3 @@
+
+    arg_index = 0;
+    for (i = n_py_list_args; i < n_py_args; i++) {

i doesn't matter. Iterate from 0 to n_py_kw_args instead.

@@ +1016,3 @@
+    arg_index = 0;
+    for (i = n_py_list_args; i < n_py_args; i++) {
+

Declare py_arg here.

@@ +1021,3 @@
+        /* Scan 0->n_args for the keyword... might need to skip
+           some if they are OUT or AUX */
+        for (; arg_index < n_args && !py_arg; arg_index++){

Declare arg_info and arg_name here.

@@ +1026,3 @@
+            
+            arg_name = g_base_info_get_name ((GIBaseInfo *) arg_info);
+            

arg_info must be freed.

@@ +1034,3 @@
+               catch it later */
+            py_arg = Py_None;
+        }

When does PyDict_GetItemString return NULL? When the item is not found, or when there is an error? In the last case, you need to clear the error.

I'm not sure inserting None in place of the argument is the right behavior; it will count for an argument.

@@ +1037,3 @@
+
+        Py_INCREF(py_arg);
+        PyTuple_SetItem(new_py_args, cur_tuple_index++, py_arg);

Separate those two operations, and use SET_ITEM.

@@ +1038,3 @@
+        Py_INCREF(py_arg);
+        PyTuple_SetItem(new_py_args, cur_tuple_index++, py_arg);
+    }     

You should use an array of PyObject* to store arguments in order, and then create a tuple out of the non-NULL entries. Another possibility is to create a Python list, and to return a tuple using PyList_AsTuple, although this is probably less efficient.

This way, you would not need nested for loops either. The inner-most loop should be sufficient.

@@ +1068,3 @@
         _free_invocation_state (&state);
         return NULL;
+    }       

?

@@ +1072,1 @@
+    Py_DECREF(py_args);

Seems like _wrap_g_function_info_invoke has exit points that don't execute this instruction.

::: gi/pygi-invoke.h
@@ +33,3 @@
+PyObject *_wrap_g_function_info_invoke (PyGIBaseInfo *self, 
+					PyObject *py_args,
+					PyObject *py_kwargs);

Please align argument names.

::: tests/test_gi.py
@@ +1558,1 @@
+class TestKeywordCalling(unittest.TestCase):

Please move this test to test_everything.

@@ +1580,3 @@
+        except:
+            failed = True
+        self.assertTrue(failed)

Use self.assertRaises, with the exact exception you expect. Otherwise, no useful error message is displayed in case of failure.

@@ +1587,3 @@
+        except:
+            failed = True
+        self.assertTrue(failed)            

Ditto.

@@ +1595,3 @@
+            failed = True
+
+        self.assertTrue(failed)

Ditto.

Created attachment 181180 [details] [review]
[gi] Implement keyword argument handling for invoke.

This is my attempt at the patch. I looking at Zach's patch, but
it no longer applies cleanly. I also looked at the TypeErrors that
Python raises to try to duplicate that behaviour. This patch
checks the arguments from inside of _prepare_invocation_state()
so that the in, out and aux argument handling code is not
duplicated.

Rest of commit message:
Handle **kwargs from Python, so that arguments can be passed by
name and in any order. This uses the parameter names from the GIR.

When the wrong number of parameters is given, a TypeError with a
message identical to that given by the Python interpreter will be
raised.

I should mention that the test case in my patch will fail unless gobject-introspection is rebuilt. I committed a change to add a function in GIMarshallingTests with multiple input arguments.

Created attachment 181195 [details] [review]
[gi] Implement keyword argument handling for invoke.

Same as previous patch except for a few Python3 fixes.
I put PyString_* inside an #ifdef and fixed the except syntax
in the tests.

Rest of commit message:
Handle **kwargs from Python, so that arguments can be passed by
name and in any order. This uses the parameter names from the GIR.

When the wrong number of parameters is given, a TypeError with a
message identical to that given by the Python interpreter will be
raised.

Review of attachment 181195 [details] [review]:

Quick review, can do a more comprehensive one later.

::: gi/pygi-invoke.c
@@ +140,3 @@
+_check_for_unexpected_kwargs(const gchar *function_name,
+                             GSList *arg_name_list,
+                             PyObject *py_kwargs)

Align the *

@@ +147,3 @@
+    while (PyDict_Next(py_kwargs, &dict_iter_pos, &dict_key, &dict_value)) {
+        gboolean key_is_expected_arg = FALSE;
+        PyObject *kwd_str;

What's wrong with PyObject *key?

@@ +162,3 @@
+        }
+#else
+        kwd_str = PyUnicode_AsUTF8String(dict_key);

Duplicating code here, you can move up the #endif and kill the #else

@@ +169,3 @@
+
+        iter = arg_name_list;
+        while (iter != NULL) {

normally;
for (l = arg_name_list; l; l = l->next) {
    .. PyBytes_AsString(l->data) ..
}

which is arguable more readable, yes use "l" instead of "iter" to avoid confusion

@@ +180,3 @@
+        }
+
+        if (key_is_expected_arg == FALSE) {

!key_is_expected_arg

@@ +198,3 @@
+                                  GSList *arg_name_list,
+                                  PyObject *py_args, /* we own a reference */
+                                  PyObject *py_kwargs /* borrowed reference */)

Align arguments

@@ +226,3 @@
+    combined_py_args = PyTuple_New(n_expected_args);
+
+    for (Py_ssize_t i=0; i < n_py_args; i++) {

You shouldn't declare variables in here.

@@ +236,3 @@
+        PyObject *item;
+
+        arg_name = (const gchar *)g_slist_nth_data(arg_name_list, i);

this is O(N), iterate over the arg_name_list instead.

@@ +263,3 @@
+
+    /* a NULL element in the tuple means an argument is missing */
+    for (Py_ssize_t i=0; i < n_expected_args; i++) {

See above

@@ +266,3 @@
+        PyObject *item = PyTuple_GET_ITEM(combined_py_args, i);
+        if (item == NULL) {
+            PyErr_Format(PyExc_TypeError,

Missing space before (, you should be consistent with the rest of the file wrt space before ( and around =

@@ +284,3 @@
+    Py_DECREF(py_args);
+    Py_XDECREF(combined_py_args);
+    return NULL;

Merge both exit paths by setting combined_py_args to NULL in the default case

@@ +321,3 @@
+                                      state->destroy_notify_index, &arg_info);
+            arg_name = g_base_info_get_name ((GIBaseInfo *) &arg_info);
+            py_ignored_kwarg_names = g_slist_append(py_ignored_kwarg_names, (gpointer)arg_name);

Never use g_slist_append as it's O(N), use prepend() + reverse()

@@ +332,3 @@
         state->n_in_args += 1;
+        /* NULL string indicates that an arg is required, but has no keyword name */
+        py_kwarg_names = g_slist_append(py_kwarg_names, NULL);

Ditto, see above.

@@ +337,3 @@
+    if (state->is_constructor) {
+        /* NULL string indicates that an arg is required, but has no keyword name */
+        py_kwarg_names = g_slist_append(py_kwarg_names, NULL);

Ditto, see above.

@@ +359,3 @@
         if (direction == GI_DIRECTION_IN || direction == GI_DIRECTION_INOUT) {
+            const gchar *name = g_base_info_get_name ((GIBaseInfo *) state->arg_infos[i]);
+            py_kwarg_names = g_slist_append(py_kwarg_names, (gpointer)name);

Ditto, see above.

@@ +396,3 @@
+                    g_callable_info_load_arg ((GICallableInfo *) function_info, length_arg_pos, &arg_info);
+                    length_arg_name = g_base_info_get_name ((GIBaseInfo *) &arg_info);
+                    py_ignored_kwarg_names = g_slist_append(py_ignored_kwarg_names, (gpointer)length_arg_name);

Ditto, see above.

@@ +422,3 @@
+        /* remove all elements of py_ignored_kwarg_names from py_kwarg_names */
+        GSList *iter = py_ignored_kwarg_names;
+        while (iter != NULL) {

Use for, see above

@@ +426,3 @@
+            GSList *link;
+
+            data = g_slist_nth_data(iter, 0);

l->data

@@ +427,3 @@
+
+            data = g_slist_nth_data(iter, 0);
+            link = g_slist_find(py_kwarg_names, data);

O(N), can it be avoided?

Created attachment 181392 [details] [review]
[gi] Implement keyword argument handling for invoke.

I have fixed all of Johan's comments in this patch, except for
getting rid of the error label in
_py_args_combine_and_check_length(). I don't know what you mean
by merging the paths, because, yes I could set combined_py_args
to NULL, but in the error case we want to decref it, otherwise
we don't want to decref it.

I have simplified the GSList building so that it is no longer
O(N^2), and the list of ignored keywords is not needed.

Rest of commit message:
Handle **kwargs from Python, so that arguments can be passed by
name and in any order. This uses the parameter names from the GIR.

When the wrong number of parameters is given, a TypeError with a
message identical to that given by the Python interpreter will be
raised.

Review of attachment 181392 [details] [review]:

::: gi/pygi-invoke.c
@@ +50,1 @@
     GITypeInfo *return_type_info;

This review and the previous one is mostly style, I haven't looked at the actual logic closely.

@@ +155,3 @@
+           key = dict_key;
+        }
+        else {

Just add a continue; so you can kill else and #else

@@ +189,3 @@
+                                   GSList      *arg_name_list,
+                                   PyObject    *py_args,  /* we own a reference */
+_check_for_unexpected_kwargs (const gchar *function_name,

Probably worth writing a real gtk-doc comment here, it's unusual to add documentation where you added it.

@@ +217,3 @@
+    }
+
+    combined_py_args = PyTuple_New (n_expected_args);

What does combined mean? A comment here would be helpful

@@ +219,3 @@
+    combined_py_args = PyTuple_New (n_expected_args);
+
+    for (i=0; i < n_py_args; i++) {

Missing spaces around =

@@ +225,3 @@
+    }
+
+    for (i=0, l = arg_name_list; i < n_expected_args && l; i++, l = l->next) {

Ditto

@@ +244,3 @@
+            PyTuple_SET_ITEM (combined_py_args, i, item);
+        }
+        else {

Usually } else {

@@ +255,3 @@
+
+    /* a NULL element in the tuple means an argument is missing */
+    for (i=0; i < n_expected_args; i++) {

Ditto see above

@@ +270,3 @@
+    }
+
+    Py_DECREF (py_args);

Actually, Py_DECREF(py_args) can be called a bit earlier right?

Review of attachment 181392 [details] [review]:

::: gi/pygi-invoke.c
@@ +50,1 @@
     GITypeInfo *return_type_info;

This review and the previous one is mostly style, I haven't looked at the actual logic closely.

@@ +155,3 @@
+           key = dict_key;
+        }
+        else {

Just add a continue; so you can kill else and #else

@@ +189,3 @@
+_py_args_combine_and_check_length (const gchar *function_name,
+                                   GSList      *arg_name_list,
+                                   PyObject    *py_args,  /* we own a reference */

Probably worth writing a real gtk-doc comment here, it's unusual to add documentation where you added it.

@@ +217,3 @@
+    }
+
+    combined_py_args = PyTuple_New (n_expected_args);

What does combined mean? A comment here would be helpful

@@ +219,3 @@
+    combined_py_args = PyTuple_New (n_expected_args);
+
+    for (i=0; i < n_py_args; i++) {

Missing spaces around =

@@ +225,3 @@
+    }
+
+    for (i=0, l = arg_name_list; i < n_expected_args && l; i++, l = l->next) {

Ditto

@@ +244,3 @@
+            PyTuple_SET_ITEM (combined_py_args, i, item);
+        }
+        else {

Usually } else {

@@ +255,3 @@
+
+    /* a NULL element in the tuple means an argument is missing */
+    for (i=0; i < n_expected_args; i++) {

Ditto see above

@@ +270,3 @@
+    }
+
+    Py_DECREF (py_args);

Actually, Py_DECREF(py_args) can be called a bit earlier right?

Created attachment 181567 [details] [review]
[gi] Implement keyword argument handling for invoke.

I have fixed all of Johan's style comments in this patch.

Rest of commit message:
Handle **kwargs from Python, so that arguments can be passed by
name and in any order. This uses the parameter names from the GIR.

When the wrong number of parameters is given, a TypeError with a
message identical to that given by the Python interpreter will be
raised.

Created attachment 193402 [details] [review]
[gi] Support function calling with keyword arguments in invoke.

My previous patch rebased on master after the invoke-rewrite merge.

Created attachment 193546 [details] [review]
[gi] Support function calling with keyword arguments in invoke.

Updates:
* Put expected arg names in a hashtable for quick lookup
* Remember to free list and hashtable
* Combine missing argument checking into main for loop

committed.  thanks