GNOME Bugzilla – Bug 792821
g_variant_type_first() and g_variant_type_next() not usable from bindings
Last modified: 2018-05-24 20:09:38 UTC
Problem ------- GVariantType is a boxed and both g_variant_type_first() and g_variant_type_next() return with transfer-none. Which in turn requires the bindings to use g_boxed_copy(), which for GVariantType means calling g_variant_type_copy(). g_variant_type_copy() uses g_variant_type_get_string_length() for copying the object, but g_variant_type_get_string_length() returns a bogus value for the iterator returned by g_variant_type_first()/next(), so the returned instance is bogus as well. Possible hacky solution ----------------------- Add new variants of first()/next() which assume that the variant is null terminated and return transfer-full using strcpy instead of g_variant_type_get_string_length(). Alias these over the other ones for bindings. Problem is that these new functions would crash if used on a not null terminated string. But this shouldn't be the case with bindings as they always have to either copy or get a copy, which is always null terminated. Example program --------------- #include <glib.h> #include <glib.h> #include <glib-object.h> int main (int argc, char *argv[]) { const GVariantType *gv = G_VARIANT_TYPE ("(si)"); const GVariantType *temp; temp = g_variant_type_first (gv); GVariantType *copy = g_boxed_copy (G_TYPE_VARIANT_TYPE, temp); g_print ("%d, %d\n", g_variant_type_get_string_length (temp), g_variant_type_get_string_length (copy)); g_print ("%s\n", g_variant_type_dup_string (temp)); temp = g_variant_type_next(temp); g_print ("%s\n", g_variant_type_dup_string (temp)); // the copy should behave the same, but doesn't g_print ("%s\n", g_variant_type_dup_string (copy)); temp = g_variant_type_next(copy); g_print ("%s\n", g_variant_type_dup_string (temp)); return 0; }
Related PyGObject bug report: https://gitlab.gnome.org/GNOME/pygobject/issues/154
I think one possible solution would be to create a new type: GVariantTypeIter { GVariantType* type; gsize offset; } and these new methods void g_variant_type_iter_next (GVariantTypeIter* iter) GVariantType* g_varant_type_iter_get_type (GVariantTypeIter* iter) [transfer full] GVariantTypeIter* g_variant_type_get_iter (GVariantType* type); and a helper macro G_VARIANT_TYPE_ITER_INIT (GVariantType* type) to initialize stack allocated iters This solution would be very cheap to use by bindings as it doesn't require any allocations to iterate unless the user does get_type() explicitely This is also safer in C, so we might consider deprecating first/next
Created attachment 371219 [details] [review] g_variant_type_first_dup, g_variant_type_next_dup An outline of the hacky solution I mentioned above.
Review of attachment 371219 [details] [review]: I think I prefer Alberto’s suggestion. Do you see any issues with it?
(In reply to Alberto Ruiz from comment #2) > I think one possible solution would be to create a new type: > > GVariantTypeIter { > GVariantType* type; > gsize offset; > } > > and these new methods > > void > g_variant_type_iter_next (GVariantTypeIter* iter) > > GVariantType* > g_varant_type_iter_get_type (GVariantTypeIter* iter) [transfer full] > > GVariantTypeIter* > g_variant_type_get_iter (GVariantType* type); > > and a helper macro G_VARIANT_TYPE_ITER_INIT (GVariantType* type) to > initialize stack allocated iters > > This solution would be very cheap to use by bindings as it doesn't require > any allocations to iterate unless the user does get_type() explicitely It would ideally still copy the GVariantType in g_variant_type_get_iter() because it has to make sure it stays alive as long as the iter. (In reply to Philip Withnall from comment #4) > Review of attachment 371219 [details] [review] [review]: > > I think I prefer Alberto’s suggestion. Do you see any issues with it? lgtm To throw in one more alternative: There already is a g_variant_type_n_items(), we could add a function returning an array of all items, something like "GVariantType ** g_variant_type_get_items(const GVariantType *type, gsize *len))"
(In reply to Christoph Reiter (lazka) from comment #5) > To throw in one more alternative: There already is a > g_variant_type_n_items(), we could add a function returning an array of all > items, something like "GVariantType ** g_variant_type_get_items(const > GVariantType *type, gsize *len))" That would have to be (transfer full) for its return value, otherwise we’d run into the same g_boxed_copy() problem as in comment #0. The main difference I see is in the number of allocations: using an iterator, you have to allocate only the iterator structure/contents; using g_variant_type_get_items(), you have to allocate the array and its items. There aren’t going to be many elements in a GVariant tuple, typically, though, so the difference in the number of allocations is likely to be small. I like the fact that g_variant_type_get_items() is only one new bit of API, whereas adding an iterator is a number of new bits of API. I think I’m leaning towards g_variant_type_get_items() for now. Emmanuele, Alberto, what do you think?
(In reply to Philip Withnall from comment #6) > That would have to be (transfer full) for its return value, otherwise we’d > run into the same g_boxed_copy() problem as in comment #0. The main > difference I see is in the number of allocations: using an iterator, you > have to allocate only the iterator structure/contents; using > g_variant_type_get_items(), you have to allocate the array and its items. > There aren’t going to be many elements in a GVariant tuple, typically, > though, so the difference in the number of allocations is likely to be small. > > I like the fact that g_variant_type_get_items() is only one new bit of API, > whereas adding an iterator is a number of new bits of API. > > I think I’m leaning towards g_variant_type_get_items() for now. > > Emmanuele, Alberto, what do you think? I think there are other things to consider, I like the notion of trying to have people using the same APIs from C and from the bindings to remove friction when moving across languages and documenting. With the get_items proposal there would be arguments to keep first/next around in C to avoid allocations whereas with the new type you get a common API that is convenient enough and shared across languages. Also, I'm not sure we can just say "it's just a couple of allocations" without taking into account actual use cases for this API, while you're not supposed to make too many DBus calls it certainly adds an overhead if you want to automatically demarshall values on frequent calls plus some DBus APIs have rather complicated nested tuples and gvariants (NetworkManager for instance). OTOH with get_items we introduce a lot less code and no new type. I still lean towards the new type, mostly because of the consistency across bindings and C and the convenience of the new API, although it requires a slightly larger patch.
I, too, have a slight preference for the GVariantTypeIter approach for a couple of reasons: - in C (and C++ and Rust), you can put the iterator type on the stack and get away with no extra allocations - it fits the pattern for various other types in GLib/GObject/GIO The proposed iterator API has a couple of inconsistencies with other iterator types; using GHashTableIter as a model, the API would look like: - init on a pointer and pass the type immediately: ``` void g_variant_type_iter_init (GVariantTypeIter *iter, GVariantType *type); ``` - next() should provide the type: ``` gboolean g_variant_type_iter_next (GVariantTypeIter *iter, GVariantType **type); ``` With `type` as an `(out caller-allocates) (transfer full)`. This should be fine for language bindings, AFAIK. Of course the "return an array of types and then let bindings use their own iteration facilities" is kind of nice in terms of punting the potential API impedance mismatch to the developer, so I'm not completely opposed to that, even if it comes with a slight memory overhead for C developers; after all, C developers can still use extant "iterator-like" API in GVariantType.
(In reply to Emmanuele Bassi (:ebassi) from comment #8) > The proposed iterator API has a couple of inconsistencies with other > iterator types; using GHashTableIter as a model, the API would look like: > > - init on a pointer and pass the type immediately: > > ``` > void > g_variant_type_iter_init (GVariantTypeIter *iter, > GVariantType *type); > ``` > > - next() should provide the type: > > ``` > gboolean > g_variant_type_iter_next (GVariantTypeIter *iter, > GVariantType **type); > ``` > > With `type` as an `(out caller-allocates) (transfer full)`. This should be > fine for language bindings, AFAIK. +1
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/1327.