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 560567 - Generalize array support
Generalize array support
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
1.29.x
Other Linux
: Normal normal
: ---
Assigned To: Johan (not receiving bugmail) Dahlin
gjs-maint
: 584568 609283 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2008-11-12 22:17 UTC by Johan (not receiving bugmail) Dahlin
Modified: 2012-03-01 16:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (4.71 KB, patch)
2008-11-12 22:22 UTC, Johan (not receiving bugmail) Dahlin
needs-work Details | Review
patch v2 (10.57 KB, patch)
2008-11-13 17:22 UTC, Johan (not receiving bugmail) Dahlin
none Details | Review
v3: (12.73 KB, patch)
2008-11-15 14:03 UTC, Johan (not receiving bugmail) Dahlin
needs-work Details | Review
v4: (19.08 KB, patch)
2008-12-20 20:41 UTC, Johan (not receiving bugmail) Dahlin
needs-work Details | Review
Generalise array support (19.45 KB, patch)
2010-02-09 10:57 UTC, Chris Lord
none Details | Review
Generalise array support (and special case GValue) (28.20 KB, patch)
2010-02-10 00:34 UTC, Chris Lord
none Details | Review

Description Johan (not receiving bugmail) Dahlin 2008-11-12 22:17:40 UTC
The array support shouldn't be hardcoded to only support strings, gjs should allow all other types, including recursive arrays.
Comment 1 Johan (not receiving bugmail) Dahlin 2008-11-12 22:22:48 UTC
Created attachment 122530 [details] [review]
patch

Generalize it, enough to support GTypes. Test added for that, and Gtk.ListStore.set_column_types() also works now.
Comment 2 Havoc Pennington 2008-11-13 04:07:36 UTC
Great, thanks,

    TYPE_STRING = 64;
    TYPE_GINT = 64;

This is a bit confusing... are those the real GType values? (I guess not if they are both 64)

 arguments = g_new0(GArgument, length+1);

Should the function still return void* if it will always be GArgument*

  g_free(arguments);

When this is freed, you need to release each individual GArgument* as well, right?

         for (i = 0; i < g_type_info_get_array_length (type_info); i++)
             {

Brace shouldn't be on its own line

                            /* no way to recover here, and errors should
                             * not be possible.
                             */

Should release_arg_internal() return a boolean if it can never really fail?

Comment 3 Tommi Komulainen 2008-11-13 08:18:17 UTC
Comment on attachment 122530 [details] [review]
patch

>+function testGType() {
>+    TYPE_STRING = 64;
>+    TYPE_GINT = 64;
>+    assertEquals(1, Everything.test_gtype_in(1, [TYPE_STRING]));
>+    assertEquals(2, Everything.test_gtype_in(2, [TYPE_STRING, TYPE_GINT]));
>+}
[...]
>+    case GI_TYPE_TAG_GTYPE:
>     case GI_TYPE_TAG_INT16: {
>         gint32 i;
>         if (!JS_ValueToInt32(context, value, &i))

Hmm, I think we need to figure out how to export GTypes, both builtin and dynamic ones I guess. IMO should deal with it separately and use strings/ints/booleans/arrays for testing the array stuff here rather than sneak int-to-gtype conversion in. (Besides, GType is sizeof(long) or so, so treating it like INT16 is going to be problematic.)


>-    if (element_type == GI_TYPE_TAG_UTF8) {
>-        return gjs_array_to_strv (context, array_value, length, arr_p);

gjs_array_to_strv() is now unused and could be deleted as well?
Comment 4 Johan (not receiving bugmail) Dahlin 2008-11-13 17:22:20 UTC
Created attachment 122589 [details] [review]
patch v2

With comments above addressed:
- gjs_array_to_stringv removed, this turned out to be non-trivial, I had to change some apis to allow this
- GType removed, I'll handle that in another bug.
- gjs_array_to_array should be leak free now, gjs_g_arg_release should do the right thing and release both the list and all individual arguments in the normal and error code path.
- gjs_arg_release_internal can currently only fail if you pass in a bad type tag, not sure if it make sense to change it to raise exceptions in case of errors, it's probably not something you'd like to catch in user code. Should either way to be done in another bug.
Comment 5 Johan (not receiving bugmail) Dahlin 2008-11-15 14:03:04 UTC
Created attachment 122729 [details] [review]
v3:

The old patch didn't actually work for arrays with more than one value.
I've reworked it to allocate the right amount of memory and added proper tests for strv, int, float and double.
This patch requires some modifications to gobject-introspection I haven't committed yet. I'm wondering if it's okay to implicitly use the ffi api.
Comment 6 Johan (not receiving bugmail) Dahlin 2008-11-19 17:23:33 UTC
(In reply to comment #5)
> Created an attachment (id=122729) [edit]
> v3:
> 
> The old patch didn't actually work for arrays with more than one value.
> I've reworked it to allocate the right amount of memory and added proper tests
> for strv, int, float and double.
> This patch requires some modifications to gobject-introspection I haven't
> committed yet. I'm wondering if it's okay to implicitly use the ffi api.
> 

This patch leaks and triggers a couple of valgrind warnings/errors. I have a new patch on another computer which fixes that with the exception that string array arguments still leaks. I think it would make sense for that patch to go in, and fix the string array leaking in another bug as the patch is already growing pretty big.
Comment 7 Tommi Komulainen 2008-12-19 11:03:20 UTC
Comment on attachment 122729 [details] [review]
v3:

The patch no longer applies as r111 renamed and made gjs_value_to_g_arg_with_type_info public
Comment 8 Johan (not receiving bugmail) Dahlin 2008-12-20 20:41:51 UTC
Created attachment 125065 [details] [review]
v4: 

This is the latest patch I found in my tree, needs to be ported to current trunk.
Comment 9 Johan (not receiving bugmail) Dahlin 2010-02-08 20:04:55 UTC
*** Bug 609283 has been marked as a duplicate of this bug. ***
Comment 10 Johan (not receiving bugmail) Dahlin 2010-02-08 20:07:12 UTC
*** Bug 584568 has been marked as a duplicate of this bug. ***
Comment 11 Chris Lord 2010-02-09 10:57:58 UTC
Created attachment 153318 [details] [review]
Generalise array support

Here's my best first-effort at updating this patch to apply against master. Note, that arrays of GValue still don't seem to work, so I've most likely gotten something wrong - I've not spent any time debugging or testing this, beyond just getting it to build, but do intend to do so.

I'd really appreciate the comment/review of the original patch author, it was a bit of a task getting this to apply to master and I may have gotten some things wrong.
Comment 12 Johan (not receiving bugmail) Dahlin 2010-02-09 13:42:56 UTC
Chris: I can't really remember all the details. But I'll be happy as long as you include unittests for all the relevant features and make sure that it doesn't introduce any additional memory leaks (check using valgrind).
Comment 13 Chris Lord 2010-02-09 14:29:17 UTC
With this code, GValue arrays end up getting returned as arrays to pointers of GValues.

It seems like we'll need to special-case GValue for this to work, does that sound correct/reasonable?
Comment 14 Johan (not receiving bugmail) Dahlin 2010-02-09 18:28:52 UTC
Special casing GValue sounds fine to me.
Comment 15 Chris Lord 2010-02-10 00:34:16 UTC
Created attachment 153376 [details] [review]
Generalise array support (and special case GValue)

This patch is an updated version of #153318 that fixes the call of gjs_array_to_array from gi/value.c (probably) and also special cases GValue in gjs_array_to_array so that arrays of GValues as arguments work correctly.

I'm not sure if the problem happens just with GValues, or if arrays of all structs as arguments need to be special-cased in this way, but this at least makes clutter_actor_animatev work correctly.

Again, I've only done very limited testing on this, so any feedback would be greatly appreciated - I don't have as much time as I'd like to work on this :(

I'll try to add/run any necessary test cases asap.
Comment 16 Chris Lord 2010-02-10 13:37:08 UTC
I'd quite like to push this to a branch if possible - I have a gnome account already, would that be ok? Any suggested branch names?
Comment 17 Chris Lord 2010-02-16 08:48:05 UTC
I've pushed this into the branch 'generalised-array-args'.
Comment 18 Morten Mjelva 2010-05-13 20:36:24 UTC
hi

is anyone still working on this?
Comment 19 Cosimo Cecchi 2011-07-06 19:12:05 UTC
At least the GValue parsing part of this bug is still relevant with latest GJS master, and makes e.g. gtk_list_store_set_valuesv() impossible to use from JS.
Comment 20 Giovanni Campagna 2012-03-01 16:56:09 UTC
We implement every kind of array known to gobject-introspection (including flat GValue arrays) nowadays, and gtk_list_store_set_valuesv works. I'd say this is fixed.

This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.