GNOME Bugzilla – Bug 582228
Convert arrays of lists/hashes/arrays from javascript to C.
Last modified: 2018-01-21 05:16:15 UTC
This code adds to gjs_array_to_array to support nested array types: arrays of lists/hashes/arrays where the elements are of pointer type. The implementation is straightforward; we just delegate each element via gjs_value_to_g_argument.
Created attachment 134436 [details] [review] Convert-arrays-of-pointers-from-javascript-arrays.patch
Created attachment 134893 [details] [review] Convert-arrays-of-pointers-from-javascript-arrays.patch Update to git HEAD. In particular, update to match 4a84c965b59941c8f8ce00309d4451a2dfa89d74
Review, please?
Comment on attachment 134893 [details] [review] Convert-arrays-of-pointers-from-javascript-arrays.patch seems that this leaks, the results is never freed right? Have you verified with valgrind.
Hm, you're right. This is a deep problem: gjs_g_arg_release_internal doesn't necessarily know the length of the array, which makes it very difficult to properly free the elements. I've attached a slightly updated patch which demonstrates the problem. I guess I can hide the array length in array[-1], say, and then use it to properly free the array. Any better ideas? (GArray support in bug 581687 is much cleaner!)
Created attachment 135460 [details] [review] LEAKY! Convert-arrays-of-pointers-from-javascript-arrays.patch
A different general approach would be to keep a "cleanup list" to do after invocation - with cleanups being something like: struct _Cleanup { enum { CLEANUP_G_FREE, CLEANUP_G_OBJECT_UNREF, ... } type; gpointer data; }
A radical version of this suggestion would be to move *all* the release processing into a linked list of clean up functions. So we wouldn't handle TRANSFER_EVERYTHING / TRANSFER_NOTHING post-invocation, we'd handle it by adding or not adding a clean up handler to the list when we marshal the argument. If the list is ordered, we could also support TRANSFER_CONTAINER nicely: when we create the contained objects we add them to the clean up list depending on the transfer setting, and then the clean up for the container need only free the container; the elements should have already been freed (or not) by that point.
Yeah, I was thinking of it as comprehensive. (I thought of this approach earlier when looking at the TRANSFER_CONTAINER case) Do you actually need to worry about ordering for the container case? It seems that it will work fine if you free the container first or second, as long as you aren't counting on the container to get to the contained elements.
You're right, ordering shouldn't matter unless you mind being given a pointer to a container with already-freed elements. You'd still need type-driven release functions for TRANSFER_EVERYTHING out-arguments, so maybe this patch wouldn't be all that big: CLEANUP_BY_TYPE (which would invoke gjs_g_arg_release) would probably suffice in most places. I've opened bug 584047 for this refactoring, and made this bug depend on that one; it should be easy to fix the leak in this patch once we can use a cleanup list.
Comment on attachment 135460 [details] [review] LEAKY! Convert-arrays-of-pointers-from-javascript-arrays.patch Marking needs-work as discussed.
As far as I could tell when I tried to rebase that patch, this has already been implemented in a different way in the meantime.