GNOME Bugzilla – Bug 646632
Support in arrays of any type
Last modified: 2011-06-16 20:40:14 UTC
Currently GJS supports in arrays only of the various integer types, and this is a serious limitation.
Created attachment 185042 [details] [review] Support in arrays of any type Adds support for converting JS Arrays to C arrays of any type, not just integers as before.
Review of attachment 185042 [details] [review]: This patch needs tests; I'll look once we have those
Created attachment 187746 [details] [review] Add tests for complex arrays as in arguments Previously gjs supported only arrays of integers. Now that this changed, we need tests to avoid regressions. (Patch against gobject-introspection for libgimarshallingtests)
Created attachment 187747 [details] [review] Support in arrays of any type Adds support for converting JS Arrays to C arrays of any type, not just integers as before. Includes new infrastructure for releasing said arrays. Now with tests! (And a bug found :) )
Created attachment 188141 [details] [review] Support in arrays of any type Adds support for converting JS Arrays to C arrays of any type, not just integers as before. Includes new infrastructure for releasing said arrays. (I forgot to git add one bugfix)
So, this patch really provokes the issue of whether we want to expose length arguments to JS. I'd honestly really prefer we didn't, since well it's lame =)
(In reply to comment #6) > So, this patch really provokes the issue of whether we want to expose length > arguments to JS. I'd honestly really prefer we didn't, since well it's lame =) Well, we already do. It would be an API break and a major rework. Not saying it's not possible, though.
Created attachment 188308 [details] [review] Rework how special arguments are marshalled Introduce an array of param_types for special arguments (callbacks, arrays with explicit length). This means that now more than one callback is supported for each function, and you don't need to specify an explicit length for arrays. ---- This is on top of the other patch, although it mostly reworks it. In order to remove length arguments, I had to modify in various places how marshalling is implemented, and the patch is really complex. But it works (in the sense that is passes unit tests), and fixes two bugs at the same time.
Review of attachment 188141 [details] [review]: This looks fine.
Review of attachment 188308 [details] [review]: Cool; thanks a lot for working on this! But we have to decide explicitly about compatibility for 3.2. Do we bite the bullet and break any users of array+len functions?
(In reply to comment #10) > Review of attachment 188308 [details] [review]: > > Cool; thanks a lot for working on this! But we have to decide explicitly about > compatibility for 3.2. Do we bite the bullet and break any users of array+len > functions? My take is yes.
(In reply to comment #9) > Review of attachment 188141 [details] [review]: > > This looks fine. I'll hold this until the other part (gimarshallingtests) is reviewed and accepted as well. (Or is it implicitly ok?) (In reply to comment #11) > (In reply to comment #10) > > Review of attachment 188308 [details] [review] [details]: > > > > Cool; thanks a lot for working on this! But we have to decide explicitly about > > compatibility for 3.2. Do we bite the bullet and break any users of array+len > > functions? > > My take is yes. Same for me. I think we can get away with a mail to desktop-devel-list, given it was said not long ago that gjs is experimental, and currently only the shell is using it. (Actually, also the-board, but I consider that experimental and unstable as well) I mean, what are the alternatives? If we hold this off, we're either forced with suboptimal API, or we'll have the same problem in 3.4, until 4.0.
Comment on attachment 188308 [details] [review] Rework how special arguments are marshalled >-gjs_callback_trampoline_free(GjsCallbackTrampoline *trampoline) >+gjs_callback_trampoline_ref(GjsCallbackTrampoline *trampoline) can the trampoline refcounting/deglobalization be done as a separate patch? >+#if 0 > static JSBool > init_callback_args_for_invocation(JSContext *context, > Function *function, >@@ -358,6 +341,7 @@ init_callback_args_for_invocation(JSContext *context, > g_base_info_unref( (GIBaseInfo*) callback_info); > return JS_TRUE; > } >+#endif should just go? >+ in_arg_cvalues[array_length_pos].v_long = length; It seems like that wouldn't work if you were on a big-endian machine and the argument was smaller than sizeof(long).
(In reply to comment #13) > (From update of attachment 188308 [details] [review]) > >-gjs_callback_trampoline_free(GjsCallbackTrampoline *trampoline) > >+gjs_callback_trampoline_ref(GjsCallbackTrampoline *trampoline) > > can the trampoline refcounting/deglobalization be done as a separate patch? No. In order to support multiple callbacks, the patch reworks how (scope call) callbacks are freed, and this refcounting is required. On the other hand, to use refcounting with current style of creating and freeing trampolines, it would be new code, to be deleted by the next commit. > >+#if 0 > > static JSBool > > init_callback_args_for_invocation(JSContext *context, > > Function *function, > >@@ -358,6 +341,7 @@ init_callback_args_for_invocation(JSContext *context, > > g_base_info_unref( (GIBaseInfo*) callback_info); > > return JS_TRUE; > > } > >+#endif > > should just go? Yes, probably. > >+ in_arg_cvalues[array_length_pos].v_long = length; > > It seems like that wouldn't work if you were on a big-endian machine and the > argument was smaller than sizeof(long). Ok. So I need to use gjs_value_to_g_argument, I suppose.
Created attachment 189026 [details] [review] Rework how special arguments are marshalled Introduce an array of param_types for special arguments (callbacks, arrays with explicit length). This means that now more than one callback is supported for each function, and you don't need to specify an explicit length for arrays.
Created attachment 189028 [details] [review] Rework how special arguments are marshalled Introduce an array of param_types for special arguments (callbacks, arrays with explicit length). This means that now more than one callback is supported for each function, and you don't need to specify an explicit length for arrays. (Forgot to remove commented out code)
sorry, should have pointed out this line too: + length = in_arg_cvalues[array_length_pos].v_long; same problem, opposite direction.
Created attachment 189087 [details] [review] Rework how special arguments are marshalled Introduce an array of param_types for special arguments (callbacks, arrays with explicit length). This means that now more than one callback is supported for each function, and you don't need to specify an explicit length for arrays.
Created attachment 189109 [details] [review] whitespace fixes for "support in arrays" (fixups for gjs style)
Created attachment 189110 [details] [review] Rework how special arguments are marshalled (rebased for the previous patch)
Created attachment 189111 [details] [review] whitespace fixes for "rework..."
OK, I support pushing this out and breaking things too. About to attach patches to bug 651558 depending on these
Hm. Actually, with just the first patch applied, things are fine, but with the second (rework) patch applied, gnome-shell crashes almost instantly at startup inside the garbage collector.
Created attachment 189122 [details] [review] Support in arrays of any type Adds support for converting JS Arrays to C arrays of any type, not just integers as before. Includes new infrastructure for releasing said arrays. Whitespace changes squashed.
Created attachment 189123 [details] [review] Rework how special arguments are marshalled Introduce an array of param_types for special arguments (callbacks, arrays with explicit length). This means that now more than one callback is supported for each function, and you don't need to specify an explicit length for arrays. Whitespace changes squashed. Two bug fixed, and valgrind checked for memory leaks.
Review of attachment 189122 [details] [review]: I'd like to see some tests using arrays of enums, and well generally more tests, but this is looking like a good start. ::: gi/arg.c @@ +2749,3 @@ + GITypeTag type_tag; + + if (transfer != GI_TRANSFER_NOTHING) Shouldn't that be == ?
(In reply to comment #26) > Review of attachment 189122 [details] [review]: > > I'd like to see some tests using arrays of enums, and well generally more > tests, but this is looking like a good start. Ok. I'll prepare some more. > ::: gi/arg.c > @@ +2749,3 @@ > + GITypeTag type_tag; > + > + if (transfer != GI_TRANSFER_NOTHING) > > Shouldn't that be == ? No. This is for (in) arguments, for which: GI_TRANSFER_NOTHING means that the callee has duped/reffed the arguments, so we need to release our temporary copies. GI_TRANSFER_CONTAINER is not supported for in arguments (and will throw before calling the C function) GI_TRANSFER_EVERYTHING means that the callee has taken ownership, so we have nothing to do.
Created attachment 189735 [details] [review] Add tests for complex arrays as in arguments Previously gjs supported only arrays of integers. Now that this changed, we need tests to avoid regressions. Added tests for arrays of enums and GSList, for having the length before the array and for lengths that are not gint.
Created attachment 189736 [details] [review] Support in arrays of any type Adds support for converting JS Arrays to C arrays of any type, not just integers as before. Includes new infrastructure for releasing said arrays. Updated for new tests
Created attachment 189737 [details] [review] Rework how special arguments are marshalled Introduce an array of param_types for special arguments (callbacks, arrays with explicit length). This means that now more than one callback is supported for each function, and you don't need to specify an explicit length for arrays.
Comment on attachment 189735 [details] [review] Add tests for complex arrays as in arguments Attachment 189735 [details] pushed as b62e22c - Add tests for complex arrays as in arguments
Comment on attachment 189736 [details] [review] Support in arrays of any type Attachment 189736 [details] pushed as f60d0a1 - Support in arrays of any type
Review of attachment 189737 [details] [review]: So I like the idea of this, it's really hard to review though because it touches so much complicated code. Some high level things. * I definitely see memory leaks running "make valgrind-check". * I don't see how the multiple callback thing can work for the unfortunate case where you have two callbacks but only one user_data (for example, g_bus_watch_name_on_connection() ). * The cleanup path is even more complex now; I wonder if we can have a mechanism where we push stuff onto a "cleanup stack" rather than having two code paths to loop over the arguments This is an overall improvement though, and my intuition here is to put it in and work on the memory leaks and cleanups after it goes in.
Attachment 189737 [details] pushed as 7ed2e7d - Rework how special arguments are marshalled