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 651558 - array+length return values
array+length return values
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
Depends on: 646632
Blocks:
 
 
Reported: 2011-05-31 15:37 UTC by Dan Winship
Modified: 2011-06-17 15:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gimarshallingtests: add a few more array tests (3.77 KB, patch)
2011-06-02 19:19 UTC, Dan Winship
none Details | Review
gi: simplify gjs_invoke_c_function() argument bookkeeping (17.35 KB, patch)
2011-06-02 19:20 UTC, Dan Winship
none Details | Review
gi: implement array+length for return value, out, and inout (13.38 KB, patch)
2011-06-02 19:20 UTC, Dan Winship
none Details | Review
gimarshallingtests: add a few more array tests (5.24 KB, patch)
2011-06-03 15:26 UTC, Dan Winship
reviewed Details | Review
gi: simplify gjs_invoke_c_function() argument bookkeeping (17.88 KB, patch)
2011-06-03 15:27 UTC, Dan Winship
none Details | Review
gi: implement array+length for return value, out, and inout (18.46 KB, patch)
2011-06-03 15:28 UTC, Dan Winship
none Details | Review
gimarshallingtests: add a few more array tests (3.88 KB, patch)
2011-06-10 18:10 UTC, Dan Winship
committed Details | Review
Handle inout array/length pairs more correctly (2.76 KB, patch)
2011-06-16 23:52 UTC, Colin Walters
committed Details | Review
testGIMarshalling: Comment out nonexistent test cases (1.79 KB, patch)
2011-06-16 23:52 UTC, Colin Walters
none Details | Review
gi: simplify gjs_invoke_c_function() argument bookkeeping (17.88 KB, patch)
2011-06-17 13:02 UTC, Dan Winship
committed Details | Review
gi: implement array+length for return value, out, and inout (18.05 KB, patch)
2011-06-17 13:02 UTC, Dan Winship
committed Details | Review

Description Dan Winship 2011-05-31 15:37:12 UTC
Given a method like:

    public string[] get_groups () {

vala generates:

    gchar** caribou_keyboard_model_get_groups (CaribouKeyboardModel* self, int* result_length1)

aka:

   <method c:identifier="caribou_keyboard_model_get_groups" name="get_groups">
    <parameters>
     <parameter direction="out" name="result_length1" transfer-ownership="none">
      <type c:type="gint" name="gint"/>
     </parameter>
    </parameters>
    <return-value transfer-ownership="full">
     <array length="0">
      <type c:type="gchar*" name="utf8"/>
     </array>
    </return-value>
   </method>

gjs cannot currently handle this, and given the current code organization, there's no obvious way to fix it; gjs_value_from_g_argument() is called to process the <return-value>, and can see the length="0", but it has no way to access the argument list to extract that value.

Thoughts?
Comment 1 Colin Walters 2011-05-31 15:45:56 UTC
See also https://bugzilla.gnome.org/show_bug.cgi?id=646632
Comment 2 Dan Winship 2011-05-31 17:31:09 UTC
OK... so I guess we'd want

  JSBool gjs_value_from_explicit_array (JSContext  *context,
                                        jsval      *value_p,
                                        GITypeInfo *type_info,
                                        GArgument  *arg,
                                        int         length);

(which would just be another small wrapper around gjs_array_from_carray_internal()). And then gjs_invoke_c_function() would notice that the return value was an array-with-length-arg, extract the length, and call gjs_value_from_explicit_array() instead of gjs_value_from_g_argument(). And suppress the out length arg.
Comment 3 Dan Winship 2011-06-02 19:19:50 UTC
Created attachment 189113 [details] [review]
gimarshallingtests: add a few more array tests

Add some tests with parameters on either side of an out array length
parameter, to ensure that bindings that omit the length parameter
don't mess up any other parameters.
Comment 4 Dan Winship 2011-06-02 19:20:16 UTC
Created attachment 189114 [details] [review]
gi: simplify gjs_invoke_c_function() argument bookkeeping

Make the out_arg_cvalues and inout_original_arg_cvalues arrays be the
same length as in_arg_cvalues, so that we don't need to keep a
separate counter for each. Rename the counter and length variables to
make it clearer what each one is counting.
Comment 5 Dan Winship 2011-06-02 19:20:20 UTC
Created attachment 189115 [details] [review]
gi: implement array+length for return value, out, and inout
Comment 6 Dan Winship 2011-06-03 15:26:35 UTC
Created attachment 189162 [details] [review]
gimarshallingtests: add a few more array tests

Add some tests with parameters on either side of an out array length
parameter, to ensure that bindings that omit the length parameter
don't mess up any other parameters.

Add some tests with returning arrays (with lengths) of structs, to
test the transfer/cleanup in that case.
Comment 7 Dan Winship 2011-06-03 15:27:23 UTC
Created attachment 189163 [details] [review]
gi: simplify gjs_invoke_c_function() argument bookkeeping

rebase for Giovanni's latest patch
Comment 8 Dan Winship 2011-06-03 15:28:29 UTC
Created attachment 189164 [details] [review]
gi: implement array+length for return value, out, and inout

rebase and add gjs_g_argument_release_out_array() (along with marshalling
tests to exercise it)
Comment 9 Colin Walters 2011-06-10 18:04:53 UTC
Review of attachment 189162 [details] [review]:

::: tests/gimarshallingtests.c
@@ +1219,3 @@
+const gint *
+gi_marshalling_tests_array_return_etc (gint first, gint *length, gint last, gint *sum)
+{

I'd prefer an explicit (out) annotation for @length.  Actually it'd be nice to at least stub out the gtk-doc parameters, like:

@first:
@length: (out):
@last:
...etc

This applies for the rest of the functions below too.
Comment 10 Dan Winship 2011-06-10 18:10:06 UTC
Created attachment 189657 [details] [review]
gimarshallingtests: add a few more array tests

Add some tests with parameters on either side of an out array length
parameter, to ensure that bindings that omit the length parameter
don't mess up any other parameters.
Comment 11 Colin Walters 2011-06-10 18:13:20 UTC
Review of attachment 189657 [details] [review]:

LGTM
Comment 12 Colin Walters 2011-06-16 23:52:35 UTC
Created attachment 190083 [details] [review]
Handle inout array/length pairs more correctly

In particular if we saw "null" for an inout array, we need to
also pass null for the length argument.
Comment 13 Colin Walters 2011-06-16 23:52:38 UTC
Created attachment 190084 [details] [review]
testGIMarshalling: Comment out nonexistent test cases
Comment 14 Dan Winship 2011-06-17 13:02:03 UTC
Created attachment 190113 [details] [review]
gi: simplify gjs_invoke_c_function() argument bookkeeping

rebase
Comment 15 Dan Winship 2011-06-17 13:02:52 UTC
Created attachment 190114 [details] [review]
gi: implement array+length for return value, out, and inout

rebase, remove erroneous tests from a bad rebase from an old version of
Giovanni's patch
Comment 16 Colin Walters 2011-06-17 14:43:24 UTC
Ok, I didn't review each individual change here in close detail; they look right at a high level.  I'm going to focus now on fixing leaks in valgrind and cleaning up the current code to understand it better.
Comment 17 Colin Walters 2011-06-17 15:04:54 UTC
Attachment 190083 [details] pushed as c501e3d - Handle inout array/length pairs more correctly
Attachment 190113 [details] pushed as a964d78 - gi: simplify gjs_invoke_c_function() argument bookkeeping
Attachment 190114 [details] pushed as 0cd4538 - gi: implement array+length for return value, out, and inout