GNOME Bugzilla – Bug 560808
Structure support
Last modified: 2008-11-19 22:39:56 UTC
Here's are patches that adds support for reading and writing fields for boxed structures (only fields that have no memory management can be written, of course.) The support depends upon g_field_info_[set/get]_field() - see bug 552371. tests/js/testEverythingEncapsulated.js is meant to hold tests of - Structures - Unions - Boxed And it should get bigger as field support is extended, even though it has only one tiny test in it now.) (I have some plans of renaming boxed.c to encapsulated.c and making it deal with non-boxed structures as well.)
Created attachment 122663 [details] [review] Generalize argument functions to allow using for fields This patch slightly modifies the functions in arg.h to allow using for field storage as well. It does a general rename of ..._g_arg_... to ..._g_argument_... to make it clear that what's being referred to is GArgument not GIArgInfo. gjs_value_to_g_arg_with_type_info() is exposed and renamed to gjs_value_to_g_argument and the boolean argument is changed to an enum (should improve the error messages a bit for existing uses.) gjs_value_to_g_arg is renamed to gjs_value_to_arg() since it *is* specific to GIArgInfo.
Created attachment 122664 [details] [review] Simple field supported for boxed types * gi/boxed.c: Define properties for the fields defined in the introspection information. * test/js/testEverythingEncapsulated.js: New field for tests of boxed types and structures.
I'm going to generalize this bug to cover a bunch more aspects of structure support along the lines discussed in: http://mail.gnome.org/archives/language-bindings/2008-November/msg00002.html The two main things that I can think of which aren't handled after these patches are: * Passing hashes directly as function parameters - you can't write: obj.set_rectangle({ x: 0, y: 0, width: 10, height: 10 }); You need to write: obj.set_rectangle(new Gdk.Rectangle({ x: 0, y: 0, width: 10, height: 10 })); (The former does work for assignment of nested structure fields.) * out and in/out structure parameters my_object_get_rectangle(MyObject *obj, GdkRectangle *out);
Created attachment 122895 [details] [review] Allow direct allocation of "simple" boxed structures. gi/boxed.c: Identify boxes types that are "simple" (no pointer fields) and allow allocating them with g_slice_new rather than requiring a constructor. test/js/testEverythingEncapsulated.js: Add a test for simple boxed structures. https://bugzilla.gnome.org/show_bug.cgi?id=560808
Created attachment 122896 [details] [review] Fix style in testEverythingEncapsulated.js Fix indentation and add missing 'let'. https://bugzilla.gnome.org/show_bug.cgi?id=560808
Created attachment 122897 [details] [review] Allow access to nested structures gi/boxed.c: Allow boxed objects that have no memory of their own but point into the memory of a parent object. Use that to implementing reading a field that is a nested simple boxed structure. (The restriction to boxed structures and simple structures is artificial and could be lifted.) test/js/testEverythingEncapsulated.js: Test access to nested structures. https://bugzilla.gnome.org/show_bug.cgi?id=560808
Created attachment 122898 [details] [review] Allow "copy construction" of boxed types gi/boxed.c: Allow a single argument to be passed to the constructor, which can be: - Another object of the same type (copied with g_boxed_copy(), or, as proofing against future support for non-boxed structures, memcpy) - A hash table of properties to set as fields of the object. test/js/testEverythingEncapsulated.js: Test copy construction https://bugzilla.gnome.org/show_bug.cgi?id=560808
Created attachment 122899 [details] [review] Allow direct assignment to nested structure fields gi/boxed.c: Allow assigning to a nested struct field from a Boxed object of the right type or from a hash of field properties. test/js/testEverythingEncapsulated.js: Test assignment to nested structure fields. https://bugzilla.gnome.org/show_bug.cgi?id=560808
Created attachment 122900 [details] [review] Support simple structures not registered as boxed Allow creating a "Boxed" object for a simple structure that is not registered as a GBoxed. Renames: gjs_g_boxed_from_boxed() => gjs_c_struct_from_boxed() gjs_boxed_from_g_boxed() => gjs_boxed_from_c_struct() gjs_g_boxed_from_union() => gjs_c_struct_from_union() gjs_union_from_g_boxed() => gjs_union_from_c_struct() Change from passing GType to _from_g_boxed() to passing GI[Struct|Union]Info to _from_c_struct() gi/boxed.c: Allow for g_registered_type_info_get_g_type() returning G_TYPE_NONE. gi/union.c: Add check (for now) that the union type is registered as a GBoxed before registering the JS class. gi/arg.c: Check whether the GIBaseInfo STRUCT/BOXED/UNION before getting the GType and making a decision based on that. test/js/testEverythingEncapsulated.js: Add tests for simple structures https://bugzilla.gnome.org/show_bug.cgi?id=560808
Created attachment 122901 [details] [review] Allow enum and flag structure fields With gobject-introspection fixed to read and write enum and flags fields (bug 561296), we can allow them as part of simple value structures. gi/boxed.c: Classify enum and flags fields as simple and when reading and writing them, go through the normal g_field_info_set/get_field code path, rather than the "nested_interface" code path. test/js/testEverythingEncapsulated.js: Test enum fields in structures and boxed types. https://bugzilla.gnome.org/show_bug.cgi?id=560808
Created attachment 123042 [details] [review] Revised - Simple field support for boxed types Here's a new version of the second patch that fixes some GInfo memory leaks and a stray include of a non-existent file.
Comment on attachment 123042 [details] [review] Revised - Simple field support for boxed types >+ test/js/testEverythingEncapulated.js \ The stray include?
The stray include was: +#include "fields.h" (The code in gobject-introspection/girepository/gfield.c was in gjs in an early version of the work.)
Comment on attachment 122895 [details] [review] Allow direct allocation of "simple" boxed structures. can_allocate_directly feels a bit like it should be precomputed in the typelib, but I guess that's a future elaboration. Looks good.
Comment on attachment 122897 [details] [review] Allow access to nested structures is "simple" really a better name than "can_allocate_directly" ? it seems very vulnerable to "creeping meaning," where maybe it's used for two things (here it already is - "can be nested by value" and "can be allocated without special allocator"). A good name could be "plain old data" and the meaning is "you could copy this thing by C assignment operator"? don't know how many people know what "POD" refers to though. anyway, simple is not satisfying. Maybe we think about it. What keeps someone from doing: let b; { let a = new A(); b = a.nested; } And then 'a' gets collected and b is dangling?
Comment on attachment 122898 [details] [review] Allow "copy construction" of boxed types Why are some of these functions taking a "jsval*" instead of "jsval" when they don't assign to *value? + if (!strcmp(g_base_info_get_name((GIBaseInfo*) priv->info), + g_base_info_get_name((GIBaseInfo*) source_priv->info)) == 0) Mixing two idioms here? (!strcmp and strcmp==0) get_field_map mixes declaration and assignment of result boxed_init_from_prop() may not guarantee an exception is set if it returns false, not sure. SpiderMonkey has flaky and inconsistent semantics for when it sets exceptions it seems like. The "success" variable here is pointless because you don't fall through to "out:" in the success case.
Comment on attachment 122899 [details] [review] Allow direct assignment to nested structure fields + /* First see if we can directly copy from the source object; It says "first" but it never tries anything else?
Comment on attachment 122900 [details] [review] Support simple structures not registered as boxed Just kind of skimmed this one, hard to pick out the changes from the renaming, but looks OK.
(In reply to comment #14) > (From update of attachment 122895 [details] [review] [edit]) > can_allocate_directly feels a bit like it should be precomputed in the typelib, > but I guess that's a future elaboration. Looks good. The whole "can allocate directly" concept is "evolving" at the current time... I don't think it is actually computable from the field signature. Things that aren't all value types, like, say: struct _GtkTargetEntry { gchar *target; guint flags; guint info; }; can actually be allocated directly fine (with some obvious care on the usage so that a directly allocated copy has a char * owned by the language binding.) But other hand, you could have a GBoxed where the visible fields were ints, but it was cast internally to a structure with extra fields. And that would have to be marked by an annotation. So, it wasn't clear to me exactly what should go into gobject-introspection. (In reply to comment #16) > (From update of attachment 122898 [details] [review] [edit]) > Why are some of these functions taking a "jsval*" instead of "jsval" when they > don't assign to *value? Combination of being freaked out by your talk on Friday about relocating garbage collectors ... scared to introduce new roots on the stack; and influence of the "Setter" prototype for JS_DefineProperty, which takes a jsval *. But looking through jsapi.h, there are tons of dircet jsval in values, so I'll fix the pointless jsval *'s. > + if (!strcmp(g_base_info_get_name((GIBaseInfo*) priv->info), > + g_base_info_get_name((GIBaseInfo*) source_priv->info)) == 0) > > Mixing two idioms here? (!strcmp and strcmp==0) Missing parantheses, though weirdly it works. Will fix to != 0. > get_field_map mixes declaration and assignment of result Will fix. (Too many different coding style...) > boxed_init_from_prop() may not guarantee an exception is set if it returns > false, not sure. SpiderMonkey has flaky and inconsistent semantics for when it > sets exceptions it seems like. What's the coding style? Just gjs_throw() always with a "Blah Failed" and count on the overwrite prevention in gjs_throw()? Note that object_instance_props_to_g_parameters() [where I borrowd code from) also doesn't gjs_throw() if JS_NextProperty() fails. Reading the JS_NextProperty() code, there is actually no way for it to fail... > The "success" variable here is pointless because you don't fall through to > "out:" in the success case. Will fix (by falling through ... I don't like duplicating cleanup code in the success and error cases.) (In reply to comment #17) > (From update of attachment 122899 [details] [review] [edit]) > + /* First see if we can directly copy from the source object; > > It says "first" but it never tries anything else? It looks right to me. if (!boxed_get_copy_source(context, proto_priv, value, &source_priv)) { JSObject *tmp_object = gjs_construct_object_dynamic(context, proto, 1, value); /* ... copy into the temp object ... */ Maybe some confusion from the fact that get_copy_source() doesn't follow the "JS_False means exception" paradigm elsewhere in the code. I'll change the comment to: /* If we can't directly copy from the source object we need * to construct a new temporary object. */
The gjs_throw() convention is "better to throw and overwrite-prevent than to not throw, but best to actually know what's going on and not throw twice" This sometimes involves reading spidermonkey code, unfortunately. The JS_GetProperty wrappers in jsapi-util.h for example exist in part to have non-annoying semantics when it comes to when they throw...
Comment on attachment 123042 [details] [review] Revised - Simple field support for boxed types This should potentially have JSPROP_ENUMERATE. Probably should: JSPROP_PERMANENT | JSPROP_SHARED
This patch was huge enough I'm sure it breaks something, but probably best to just put it all in as it stands and then we'll evolve it.
Forgot to reply to: (In reply to comment #15) > What keeps someone from doing: > let b; > { let a = new A(); b = a.nested; } > And then 'a' gets collected and b is dangling? The code uses JS_SetReservedSlot() as described in: * We allocate 1 reserved slot; this is typically unused, but if the * boxed is for a nested structure inside a parent structure, the * reserved slot is used to hold onto the parent Javascript object and * make sure it doesn't get freed. So a won't be collected until b is collected. Committed everything. I'll file enhancement bugs for the two items I described in comment #3 as to not lose track of the loose ends.