GNOME Bugzilla – Bug 652753
no support for GI_ARRAY_TYPE_PTR_ARRAY
Last modified: 2011-08-18 13:20:52 UTC
I'm trying to use tp_base_client_delegate_channels_finish [1] in gnome-shell: _delegateFinish : function (o, res, data) { try { o.delegate_channels_finish(res); } catch (e) { print ("delegate failed" + e); } }, open: function(notification) { if (this._client.is_handling_channel(this._channel)) { // We are handling the channel, try to pass it to Empathy this._client.delegate_channels_async([this._channel], global.get_current_time(), "", Lang.bind(this, this._delegateFinish)); (...) [1] http://telepathy.freedesktop.org/doc/telepathy-glib/telepathy-glib-base-client.html#tp-base-client-delegate-channels-finish calling o.delegate_channels_finish(res) raises this assertion: ERROR:gi/arg.c:2246:gjs_g_arg_release_internal: code should not be reached Here is the gir of this function: <method name="delegate_channels_finish" c:identifier="tp_base_client_delegate_channels_finish" version="0.15.0" throws="1"> <doc xml:whitespace="preserve">Finishes an async channels delegation request started using tp_base_client_delegate_channels_async(). can be used to know the channels that @self is not handling any more, otherwise %FALSE.</doc> <return-value transfer-ownership="none"> <doc xml:whitespace="preserve">%TRUE if the operation succeed, @delegated and @not_delegated</doc> <type name="gboolean" c:type="gboolean"/> </return-value> <parameters> <parameter name="result" transfer-ownership="none"> <doc xml:whitespace="preserve">a #GAsyncResult</doc> <type name="Gio.AsyncResult" c:type="GAsyncResult*"/> </parameter> <parameter name="delegated" direction="out" caller-allocates="0" transfer-ownership="container"> <doc xml:whitespace="preserve">if not %NULL, used to return a #GPtrArray containing the #TpChannel<!-- -->s which have been properly delegated</doc> <array name="GLib.PtrArray" c:type="GPtrArray**"> <type name="Channel"/> </array> </parameter> <parameter name="not_delegated" direction="out" caller-allocates="0" transfer-ownership="container"> <doc xml:whitespace="preserve">if not not %NULL, used to return a #GHashTable mapping #TpChannel<!-- -->s which have not been delegated to a #GError explaining the reason of the failure</doc> <type name="GLib.HashTable" c:type="GHashTable**"> <type name="Channel"/> <type name="GLib.Error"/> </type> </parameter> </parameters> </method>
Here is the trace.
+ Trace 227495
Created attachment 190228 [details] [review] Add support for GPtrArrays Using code that already exists for GArray, implement marshalling for GPtrArrays, including of integers (with GINT_TO_POINTER).
do you have any examples of people using GPtrArrays of ints? If you have ints, you should just use a GArray...
(In reply to comment #3) > do you have any examples of people using GPtrArrays of ints? If you have ints, > you should just use a GArray... gi_marshalling_tests_gptrarray_int_none_in is the example I found, but there could be more, considering that giscanner doesn't warn for such usage
Would be good to get this merged asap as it blocks this gnome-shell bug: bug #653620
Comment on attachment 190228 [details] [review] Add support for GPtrArrays from a quick look the code looks good, as do the various array-handling cleanups > > do you have any examples of people using GPtrArrays of ints? If you have ints, > > you should just use a GArray... > > gi_marshalling_tests_gptrarray_int_none_in is the example I found IMHO that should be removed. If you have ints, you should use a GArray. It's not like GList/GHashTable where there's no corresponding scalar-typed list/hash type and so there's *some* excuse for abusing the type system... > GByteArray *byte_array = g_byte_array_sized_new(length); > > g_byte_array_append(byte_array, data, length); >+ arg->v_pointer = byte_array; >+ >+ g_free(data); >+ } else if (array_type == GI_ARRAY_TYPE_PTR_ARRAY) { >+ GPtrArray *array = g_ptr_array_sized_new(length); >+ >+ g_ptr_array_set_size(array, length); >+ memcpy(array->pdata, data, sizeof(gpointer) * length); >+ arg->v_pointer = array; >+ > g_free(data); we should fix this at some point to let us convert the array directly into the bytearray/ptrarray so we don't have the g_free() at the end. >- // Tests disabled due to do g-i typelib compilation bug >- // https://bugzilla.gnome.org/show_bug.cgi?id=622335 >- //array = GIMarshallingTests.garray_int_none_return(); should that bug be closed?
Created attachment 191145 [details] [review] Forbid GPtrArrays holding non-pointer types It should be safe for bindings to assume that GPtrArrays hold only pointers (or values as big as it), so there is no need to go through hoops for converting smaller integers when marshalling. Libraries that need arrays of integers should use GArray.
Created attachment 191150 [details] [review] Disallow non byte types for GByteArrays Similarly to GPtrArrays, GByteArrays can only contain bytes. Emit a warning if an inconsistent (element-type) is placed, and ensure that the default is guint8 if nothing is added. This way bindings can support GByteArrays without special casing them. While we're here, we can fix this one as well, so that I can remove all code that special cased element_type.
Created attachment 191152 [details] [review] Add support for GPtrArrays Using code that already exists for GArray, implement marshalling for GPtrArrays. Includes some general cleanup of array code.
Created attachment 191153 [details] [review] Don't release too much when releasing arrays The functions that released out arrays were iterating over pointer sized values, irrespective of the actual element size. Fix that by checking that the element is actually a pointer (everything else requires no release, except for the actual array block)
Comment on attachment 191145 [details] [review] Forbid GPtrArrays holding non-pointer types >+ # GPtrArrays are allowed to contain non basic types >+ # (except enums and flags) or basic types that are >+ # as big as a gpointer >+ if array.array_type == ast.Array.GLIB_PTRARRAY and \ >+ ((array.element_type in ast.BASIC_GIR_TYPES \ >+ and not array.element_type in ast.POINTER_TYPES) or \ >+ isinstance(array.element_type, ast.Enum) or \ >+ isinstance(array.element_type, ast.Bitfield)): >+ message.warn("invalid (element-type) for a GPtrArray, " >+ "must be a pointer", options.position) I'm not sure it's safe to assume "non-basic except enum or flags" means "pointer". Colin? (Even if it is, that expression is really confusing. Maybe just add an is_pointer flag/method to Type?)
Comment on attachment 191150 [details] [review] Disallow non byte types for GByteArrays >+ if isinstance(node.type, ast.Array): >+ self._check_array_element_type(node.type, options) does this part belong in the previous patch?
Comment on attachment 191152 [details] [review] Add support for GPtrArrays >- if ((JSVAL_IS_NULL(value) && !may_be_null) || (!JSVAL_IS_STRING(value) && !JSVAL_IS_OBJECT(value))) { >+ if ((JSVAL_IS_NULL(value) && !may_be_null) || >+ (!JSVAL_IS_STRING(value) && !JSVAL_IS_OBJECT(value) && !JSVAL_IS_NULL(value))) { does this mean allow-none was previously broken for arrays? >+ /* extra reference will be removed gjs_g_argument_release */ removed BY gjs... > g_byte_array_append(byte_array, data, length); >+ arg->v_pointer = byte_array; so bytearrays were previously totally broken? (If there are actual bugs fixed here, it would be nice to split those out into a separate commit(s) rather than squashing them with the new feature and general rewrite.) >@@ -2718,11 +2660,61 @@ gjs_g_arg_release_internal(JSContext *context, >+ switch (element_type) { >+ case GI_TYPE_TAG_UINT8: >+ case GI_TYPE_TAG_UINT16: >+ case GI_TYPE_TAG_UINT32: >+ case GI_TYPE_TAG_UINT64: >+ case GI_TYPE_TAG_INT8: >+ case GI_TYPE_TAG_INT16: >+ case GI_TYPE_TAG_INT32: >+ case GI_TYPE_TAG_INT64: >+ g_ptr_array_free(array, TRUE); >+ break; unnecessary now? Or still needed for cleanup in a bad-typelib case?
Comment on attachment 191153 [details] [review] Don't release too much when releasing arrays looks ok
(In reply to comment #12) > (From update of attachment 191150 [details] [review]) > >+ if isinstance(node.type, ast.Array): > >+ self._check_array_element_type(node.type, options) > > does this part belong in the previous patch? So and so: the previous patch works without this part, because you cannot have a GPtrArray without (element-type), whereas it makes sense for GByteArray. (In reply to comment #13) > (From update of attachment 191152 [details] [review]) > >- if ((JSVAL_IS_NULL(value) && !may_be_null) || (!JSVAL_IS_STRING(value) && !JSVAL_IS_OBJECT(value))) { > >+ if ((JSVAL_IS_NULL(value) && !may_be_null) || > >+ (!JSVAL_IS_STRING(value) && !JSVAL_IS_OBJECT(value) && !JSVAL_IS_NULL(value))) { > > does this mean allow-none was previously broken for arrays? Yes. The second gjs patch adds tests for null, so we can hope it won't happen again. > > g_byte_array_append(byte_array, data, length); > >+ arg->v_pointer = byte_array; > > so bytearrays were previously totally broken? No, only converting from string and regular arrays. JS ByteArrays worked fine (that's why the tests passed). > (If there are actual bugs fixed here, it would be nice to split those out into > a separate commit(s) rather than squashing them with the new feature and > general rewrite.) Ok. > >@@ -2718,11 +2660,61 @@ gjs_g_arg_release_internal(JSContext *context, > >+ switch (element_type) { > >+ case GI_TYPE_TAG_UINT8: > >+ case GI_TYPE_TAG_UINT16: > >+ case GI_TYPE_TAG_UINT32: > >+ case GI_TYPE_TAG_UINT64: > >+ case GI_TYPE_TAG_INT8: > >+ case GI_TYPE_TAG_INT16: > >+ case GI_TYPE_TAG_INT32: > >+ case GI_TYPE_TAG_INT64: > >+ g_ptr_array_free(array, TRUE); > >+ break; > > unnecessary now? Or still needed for cleanup in a bad-typelib case? Probably unnecessary.
Created attachment 191343 [details] [review] Fix (allow-none) for arrays Previous check rejected non-string and non-object in all cases, even when null was allowed.
Created attachment 191344 [details] [review] Fix converting to GByteArrays from strings or arrays When converting an array or a string to a GByteArray (but not a JS ByteArray), it forgot to actually set the bytearray in the resulting GArgument.
Created attachment 191345 [details] [review] Add support for GPtrArrays Using code that already exists for GArray, implement marshalling for GPtrArrays. Includes some general cleanup of array code.
Comment on attachment 191345 [details] [review] Add support for GPtrArrays looks good assuming the tests do in fact pass
Attachment 191343 [details] pushed as 4f63f43 - Fix (allow-none) for arrays Attachment 191344 [details] pushed as f8a4f68 - Fix converting to GByteArrays from strings or arrays Holding back the patch for GPtrArray, pending approval on gobject-introspection
(In reply to comment #18) > Created an attachment (id=191345) [details] [review] > Add support for GPtrArrays > > Using code that already exists for GArray, implement marshalling > for GPtrArrays. Includes some general cleanup of array code. Could we please merge this? I need it to use tp_connection_dup_contact_list() which I need to fix bug #653941
Attachment 191145 [details] pushed as 8a4e168 - Forbid GPtrArrays holding non-pointer types Attachment 191150 [details] pushed as 4c90020 - Disallow non byte types for GByteArrays Nobody complained, so I'm committing these.
Attachment 191153 [details] pushed as fa8b844 - Don't release too much when releasing arrays Attachment 191345 [details] pushed as 1149483 - Add support for GPtrArrays