GNOME Bugzilla – Bug 657767
Add support for flat GValue arrays
Last modified: 2012-01-06 21:01:10 UTC
Some APIs like to use a singular, flat GValue* array for convenience.
Created attachment 195255 [details] [review] tests: Add tests for GValue
Created attachment 195256 [details] [review] Add support for flat GValue arrays
Created attachment 195431 [details] [review] tests: Add tests for GValue
Created attachment 195433 [details] [review] Add support for flat GValue arrays
look at bug 560567 — Chris Lord had a branch a year ago fixing this.
Review of attachment 195433 [details] [review]: This should be squashed with the test patch. I don't see this depending on a patch to add GIMarshallingTests.gvalue_flat_array(). So...this is changing how we handle all GValue arrays, right? I'm not sure that is_pointer is going to be false for arrays right now. So the commit title should be "Assume arrays of GValue are 'flat'." Now special casing GValue here is just perpetuating the hack for flat arrays. It's really a mess, I know, but...it's probably worth adding an annotation for this. Also, we should be encouraging C authors to use GValueArray, not a C array of GValue*.
(In reply to comment #6) > Review of attachment 195433 [details] [review]: > > This should be squashed with the test patch. > > I don't see this depending on a patch to add > GIMarshallingTests.gvalue_flat_array(). bug #657766. I should add a dependency. > So...this is changing how we handle all GValue arrays, right? I'm not sure > that is_pointer is going to be false for arrays right now. So the commit title > should be "Assume arrays of GValue are 'flat'." GValue** should still work because is_pointer is TRUE. We can add tests to other things for that. > Now special casing GValue here is just perpetuating the hack for flat arrays. > It's really a mess, I know, but...it's probably worth adding an annotation for > this. An annotation for what? We can pretty safely assume that a "GValue*" annotated with the (array) annotation is a flat array. > Also, we should be encouraging C authors to use GValueArray, not a C array of > GValue*.
(In reply to comment #7) > > GValue** should still work because is_pointer is TRUE. We can add tests to > other things for that. Hmm...ok. But then I'm still worried that there's code out there that is using GValue * for a non-flat array, and after this it will just start crashing. From inspecting a few random modules they all seem to be flat, so maybe it's unusual enough that we could probably get by with just changing it. > An annotation for what? We can pretty safely assume that a "GValue*" annotated > with the (array) annotation is a flat array. The general problem - what do we do with other arrays of boxed types? If a C author returns GDateTime * and says it's an array, do we also assume it's flat or not? If it's flat, do we unref individual elements? Should we just reject this? (Probably). It's definitely worth looking at Chris' patch as ebassi mentioned - but I think Giovanni did some more generalizations of the array work, so it might be mostly obsolete.
Created attachment 204637 [details] [review] Add support for flat GValue arrays Tests are squashed. We also support flat GValue array returns, although this made the code a bit more complicated.
Review of attachment 204637 [details] [review]: Looks OK in general. ::: gi/arg.c @@ +2729,3 @@ + g_free(arg->v_pointer); + return JS_TRUE; + } I think you shouldn't free if transfer is GI_TRANSFER_CONTAINER, though I doubt anyone would actually return such a thing.
Attachment 204637 [details] pushed as 0688f72 - Add support for flat GValue arrays Pushed with suggested changes. Thanks for reviewing!