GNOME Bugzilla – Bug 580450
Reference counting and boxed types for arrays
Last modified: 2009-04-29 15:27:16 UTC
It would be nice if 1. GArray, GPtrArray and GByteArray had reference counting and boxed types 2. it was possible to set a GDestroyNotify on pointer arrays so when returning a GPtrArray all the user has to do to free it is g_ptr_array_unref() 3. it was possible to get the element size of a GArray I will attach a patch for doing this.
Created attachment 133420 [details] [review] Proposed patch This is similar to how GHashTable works - e.g. if you call g_array_free() on an array with reference count > 1 then the GArray wrapper is preserved but the size of the array will be set to 0.
I need this for the proposed GDBus library (bug 579571) but I think this is useful on it's own too.
This all looks great to me. But why do you 'resurrect' the array just before freeing it here ? Looking at the g_ptr_array_free code, it doesn't seem to make any difference, since the behaviour of free only depends on whether the refcount is > 1 or not. + if (g_atomic_int_dec_and_test (&array->ref_count)) + { + g_atomic_int_inc (&array->ref_count); + g_ptr_array_free (farray, TRUE); + }
(In reply to comment #3) > This all looks great to me. Great, thanks for looking at it. > But why do you 'resurrect' the array just before freeing it here ? > Looking at the g_ptr_array_free code, it doesn't seem to make any difference, > since the behaviour of free only depends on whether the refcount is > 1 or not. > > + if (g_atomic_int_dec_and_test (&array->ref_count)) > + { > + g_atomic_int_inc (&array->ref_count); > + g_ptr_array_free (farray, TRUE); > + } Yeah, that's not necessary (I just changed it and my tests in GDBus passed) - it was there when I had g_array_free() warn to stderr if the reference count was != 1. Will attach a patch without this.
Created attachment 133440 [details] [review] Updated patch
Almost good to go. Please fix the parameter names to match in headers, sources and doc comments. If they don't, like below, gtk-doc will get it wrong at some point. I also think it is confusing if the doc comments has different parameter names than the function right below it... +/** + * g_array_unref: + * @array: A #GArray. + * + * Atomically decrements the reference count of @array by one. If the + * reference count drops to 0, all memory allocated by the array is + * released. This function is MT-safe and may be called from any + * thread. + * + * Since: 2.22 + **/ +void +g_array_unref (GArray *farray)
Created attachment 133512 [details] [review] Also use @element_free_func when removing elements (In reply to comment #6) > Almost good to go. Thanks for the review. I've attached an updated patch that also uses @element_free_func on g_ptr_array_remove() and friends. We generally want this, yes? > Please fix the parameter names to match in headers, sources > and doc comments. If they don't, like below, gtk-doc will get it wrong at some > point. I also think it is confusing if the doc comments has different parameter > names than the function right below it... OK, done for the added functions.
Created attachment 133565 [details] [review] Add test cases Here's a new patch with test cases - believe it or not, the test cases uncovered a bug which is now fixed in the updated patch...
Thanks for adding the tests. Please add the new test to the Makefile target when you commit, thanks.
Created attachment 133569 [details] [review] Include boxed types Gah, I forgot the boxed types, here we go again. > Please add the new test to the Makefile target when you commit, thanks. Nothing to add, I just extended glib/tests/array-tests.c
Thanks, please commit
Committed, thanks. http://git.gnome.org/cgit/glib/commit/?id=402847c8878a6bf839facdf7a91f096769ebc609
*** Bug 508347 has been marked as a duplicate of this bug. ***