GNOME Bugzilla – Bug 590808
GKeyFile should have a refcount and a boxed type in GObject
Last modified: 2011-10-15 21:55:43 UTC
This would make it more usable directly by bindings, among other things.
I have a need for this in Evolution as well. Different parts of the application share the same GKeyFile and I'd like to be able to reference and unreference it like I do with most everything else.
Refcounting GKeyFile was rejected in bug 301034.
On grounds that aren't valid anymore. GArray, GAsyncQueue, GByteArray, GHashTable, GHook, GIOChannel, GMainContext, GMainLoop, GMappedFile, GPtrArray, GRegex, GSource and GTree are all reference counted these days.
*** Bug 615070 has been marked as a duplicate of this bug. ***
Created attachment 158129 [details] [review] [GKeyFile] Add g_key_file_copy and make it a boxed Make GKeyFile a boxed so it can be used from language bindings. https://bugzilla.gnome.org/show_bug.cgi?id=615070
Annotations for introspection bindings will go into the gobject-introspection module for now.
Although it would probably be better to make it a reference counted structure, as per bug 301034
Created attachment 158138 [details] [review] [GKeyFile] Register a boxed type Register a boxed type for the GKeyFile struct, this will make it possible to use it from language bindings. This is a simplified patch which doesn't export any new APIs, it's just exposes the current interface (copy/free) of GKeyFile to the GObject type system.
g_slice_copy() is not a valid copy func for GKeyFile. Free the original keyfile, and the copy's members will point to freed memory.
Created attachment 158230 [details] [review] [GKeyFile] Add refcounting API Based on the patch by Christian Persch and Emmanuele Bassi. Author: Christian Persch Signed-off-by: Johan Dahlin
Created attachment 158232 [details] [review] [GKeyFile] Register a boxed type Register a boxed type for the GKeyFile struct, this will make it possible to use it from language bindings.
*** Bug 301034 has been marked as a duplicate of this bug. ***
Review of attachment 158232 [details] [review]: ::: gobject/gboxed.c @@ +304,3 @@ +{ +g_key_file_get_type(void) +GType shouldn't we use GOnce for thread safety here?
Created attachment 158359 [details] [review] [GKeyFile] Register a boxed type Register a boxed type for the GKeyFile struct, this will make it possible to use it from language bindings. This is an updated patch which uses GOnce for thread-safety as suggested by Emmanuele.
Review of attachment 158230 [details] [review]: ::: glib/gkeyfile.c @@ +706,3 @@ + * Returns: the same @key_file. + * + * Since: 2.24 Since tag needs to say 2.30 by now, time flies... @@ +712,3 @@ +{ + g_return_val_if_fail (key_file != NULL, NULL); + g_return_val_if_fail (key_file->ref_count > 0, NULL); I've removed these kinds of checks for ref_count > 0 from similar places recently. They do nonatomic access, so its kinda unsafe. And using an atomic access for this seems like overkill. @@ +745,3 @@ + * reaches zero, frees the key file and all its allocated memory. + * + * Since: 2.24 2.30 here, too. @@ +751,3 @@ +{ + g_return_if_fail (key_file != NULL); + g_return_if_fail (key_file->ref_count > 0); Same here. @@ +753,3 @@ + g_return_if_fail (key_file->ref_count > 0); + + if (g_atomic_int_exchange_and_add (&key_file->ref_count, -1) - 1 == 0) Should use g_atomic_int_dec_and_test here.
Review of attachment 158359 [details] [review]: This one looks fine (of course, depends on the earlier patch)
Review of attachment 158359 [details] [review]: Oh, but it needs to get a define in glib-types.h, with a proper Since: tag.
*** Bug 652880 has been marked as a duplicate of this bug. ***
Created attachment 190450 [details] [review] GKeyFile: Add refcounting API Adds g_key_file_ref and g_key_file_unref, to be used by a future GKeyFile boxed type for language bindings. Based on the patch by Christian Persch and Emmanuele Bassi. ------ Updated to use g_atomic_int_dec_and_test. Updated Since. Removed ref_count checks. Author: Christian Persch Signed-off-by: Johan Dahlin Signed-off-by: Giovanni Campagna
Created attachment 190452 [details] [review] glib: turn GKeyFile into a boxed for introspection Using the new refcounting API, introduce a boxed type wrapping GKeyFile and expose it introspection bindings in glib-types.h
Created attachment 190453 [details] [review] GKeyFile: improve introspection annotations Ensure that all methods that take or return arrays are annotated (including length). Mark ref, unref and free methods as (skip).
Review of attachment 190450 [details] [review]: Feel free to commit after one minor change. ::: glib/gkeyfile.c @@ +90,3 @@ gchar **locales; + + volatile gint ref_count; Can you put the refcount at the start of the structure? I am thinking about adding generic "refcounted boxed" handling, and that's easy if the refcount is always at the start.
Review of attachment 190452 [details] [review]: This one needs to add to gobject.symbols too.
Review of attachment 190453 [details] [review]: This all looks good, thanks!
Given that we have already branched for 2.30, this is probably good to go in now.