GNOME Bugzilla – Bug 557383
Virtual functions not written to typelib
Last modified: 2015-02-07 16:56:27 UTC
Comparing the .gir file for foo-1.0 from g-ir-scanner to the one from g-ir-generate one can see virtual functions (callbacks) are lost: - <interface name="Interface" - c:type="FooInterface" - glib:type-name="FooInterface" - glib:get-type="foo_interface_get_type"> - <callback name="do_foo" c:type="do_foo"> - <return-value> - <type name="none" c:type="void"/> - </return-value> - <parameters> - <parameter name="self"> - <type name="Interface" c:type="FooInterface*"/> - </parameter> - </parameters> - </callback> - </interface> + <interface name="Interface" glib:type-name="FooInterface" glib:get-type="foo_interface_get_type"/>
Ok, so right now we have: <interface name="SubInterface" c:type="FooSubInterface" glib:type-name="FooSubInterface" glib:get-type="foo_sub_interface_get_type"> <prerequisite name="Interface"/> <method name="do_bar" c:identifier="foo_sub_interface_do_bar"> <return-value transfer-ownership="none"> <type name="none" c:type="void"/> </return-value> </method> <callback name="do_bar" c:type="do_bar"> <return-value transfer-ownership="none"> <type name="none" c:type="void"/> </return-value> <parameters> <parameter name="self" transfer-ownership="none"> <type name="SubInterface" c:type="FooSubInterface*"/> </parameter> </parameters> </callback> </interface> I propose to change this to: <interface name="SubInterface" c:type="FooSubInterface" glib:type-name="FooSubInterface" glib:get-type="foo_sub_interface_get_type"> <prerequisite name="Interface"/> <glib:vfunc name="do_bar" c:identifier="foo_sub_interface_do_bar"> <return-value transfer-ownership="none"> <type name="none" c:type="void"/> </return-value> </glib:vfunc> </interface> The typelib currently has the concept of both "method" and "vfunc" for classes and interfaces, we need to parse these and reflect them in the .gir correctly.
Created attachment 129593 [details] [review] Bug 557383 - Virtual function support In order to determine whether a method is virtual, by default we look at the class table to find a callback field. This should be fairly reliable, but we may also later need annotations to more finely control this in the case of a name clash with a signal.
Comment on attachment 129593 [details] [review] Bug 557383 - Virtual function support Looks god to me, thanks!
accepted-patch_is_divine
Ok, problem with this is that now these vfuncs aren't regular methods, and thus don't appear from g_object_info_find_method. We can't just make g_object_info_find_method return a vfunc, since it's not a GIFunctionInfo. In fact, strictly speaking there's not guaranteed to be a C wrapper; though right now the scanner does require it. So I think we should revert this patch, and then figure out how to make this work with a binding such as gjs. First, we need g_object_info_find_vfunc (ObjectInfo *, const char *name); From there, possibilities: Option 1) Require bindings to know how to retrieve the field offset of the vfunc, read the value there, get the signature, and invoke the vfunc. Option 2) Provide g_vfunc_info_get_function, only works if there is a C vfunc sugar. Then you can use the same code your binding uses now to invoke GIFunctionInfo. Option 3) Provide a "faked" GIFunctionInfo from g_func_info_synthesize_function that can be passed to g_function_info_invoke. Of these, I'm leaning towards 2) for now. We should double check though before actually closing this bug that a binding *could* do 1) (since it's faster and more correct). And I think right now it couldn't, since we don't write out the offset from the scanner, and the typelib loses the callback information needed to grovel through the class struct.
Created attachment 129703 [details] [review] Bug 557383 - Virtual method support Broadly speaking, this change adds the concept of <vfunc> to the .gir. The typelib already had most of the infrastructure for virtual functions, though there is one API addition. The scanner assumes that any class callback slot that doesn't match a signal name is a virtual. In the .gir, we write out *both* the <method> wrapper and a <vfunc>. If we can determine an association between them (based on the names matching, or a new Virtual: annotation), then we notate that in the .gir. The typelib gains an association from the vfunc to the function, if it exists. This will be useful for bindings since they already know how to consume FunctionInfo.
I haven't inspected the changes to most .girs yet for this. One concern I have is that the current vfunc heuristics may not be reliable enough. It looks like in gobject/gtk+ there is some convention of writing /* virtual table */ before the list of vfuncs. We may need scanner support for that (or conversely, look for /* signals */ and exclude those).
Created attachment 129805 [details] [review] Bug 557383 - Virtual method support Broadly speaking, this change adds the concept of <vfunc> to the .gir. The typelib already had most of the infrastructure for virtual functions, though there is one API addition. The scanner assumes that any class callback slot that doesn't match a signal name is a virtual. In the .gir, we write out *both* the <method> wrapper and a <vfunc>. If we can determine an association between them (based on the names matching, or a new Virtual: annotation), then we notate that in the .gir. The typelib gains an association from the vfunc to the function, if it exists. This will be useful for bindings since they already know how to consume FunctionInfo.
This new iteration adds g_object_info_find_vfunc and g_interface_info_find_vfunc, and tests for all of this in gitestrepo.c.
Comment on attachment 129805 [details] [review] Bug 557383 - Virtual method support Looks pretty good! >Broadly speaking, this change adds the concept of <vfunc> to the .gir. >The typelib already had most of the infrastructure for virtual functions, >though there is one API addition. I'm unsure about the 'vfunc' naming. There's no point in using acronyms in the gir which we want to have fairly verbose. virtual-function or virtual-method? It's actually more of a method than function right? >diff --git a/girepository/ginfo.c b/girepository/ginfo.c >+/** >+ * g_object_info_find_vfunc: >+ * @info: An #GIObjectInfo >+ * @name: The name of a virtual function to find. >+ * >+ * Locate a virtual function slot with name @name. Note that the namespace >+ * for virtuals is distinct from that of methods; there may or may not be >+ * a concrete method associated for a virtual. If there is one, it may >+ * be retrieved using #g_vfunc_info_get_invoker. See the documentation for >+ * that function for more information on invoking virtuals. >+ * >+ * Return value: A #GIVFuncInfo, or %NULL if none with name @name. >+ */ (transfer full) right? >+ return find_vfunc (base, offset, blob->n_vfuncs, name); >+/** >+ * g_interface_info_find_vfunc: >+ * @info: An #GIObjectInfo >+ * @name: The name of a virtual function to find. >+ * >+ * Locate a virtual function slot with name @name. See the documentation >+ * for #g_object_info_find_vfunc for more information on virtuals. >+ * >+ * Return value: A #GIVFuncInfo, or %NULL if none with name @name. >+ */ Ditto >+/** >+ * g_vfunc_info_get_invoker: >+ * @info: A #GIVFuncInfo >+ * >+ * If this virtual function has an associated invoker method, this >+ * method will return it. >+ * >+ * Return value: An invoker function, or %NULL if none known >+ */ Here too right? Bit unsure but you're returning a pointer to a struct which I think is allocated. >+static int >+get_index_of_member_type (GIrNodeInterface *node, GIrNodeTypeId type, const char *name) Wrap at 80. >+ invoker = find_attribute ("invoker", attribute_names, attribute_values); not sure this is the best attribute name. Any suggestions? invokation (or is it invocation?) is used in slightly different contexts in g-i. >diff --git a/tests/repository/gitestrepo.c b/tests/repository/gitestrepo.c >+ /* vfunc tests */ Perhaps turn this into a proper GTester testsuite?
Created attachment 129877 [details] [review] Bug 557383 - Virtual method support Broadly speaking, this change adds the concept of <vfunc> to the .gir. The typelib already had most of the infrastructure for virtual functions, though there is one API addition. The scanner assumes that any class callback slot that doesn't match a signal name is a virtual. In the .gir, we write out *both* the <method> wrapper and a <vfunc>. If we can determine an association between them (based on the names matching, or a new Virtual: annotation), then we notate that in the .gir. The typelib gains an association from the vfunc to the function, if it exists. This will be useful for bindings since they already know how to consume FunctionInfo.
This one fixes up all comments except the last two. The main alternatives I came up with for "invoker" were "entry point", "stub", and "c method", but they all seem more awkward to me. The invoker terminology only comes up in g-i with g_function_info_invoke, no? Whereas this use is pretty close to virtuals. As for GTester I'm in favor but we should do it in a separate commit.
Committed.
Created attachment 130776 [details] [review] A patch to include callbacks in the StructBlob in the typelib Yet there we must add API in GIStructInfo to retrieve the callback. Also gi_vfunc_info_get_signal() should return that added callback.
Created attachment 130987 [details] [review] Add a callback ref in the VFuncBlob and api to retrieve it through girepository
Have used Didier's patches as the base for my work in #560281 and they work fine as-is.
Review of attachment 130776 [details] [review]: ::: girepository/girnode.c @@ +1366,3 @@ { GIrNodeTypeId container_type) +#if 1 Should just be removed. ::: girepository/gtypelib.h @@ +682,3 @@ guint16 n_methods; guint16 n_fields; + guint16 n_callbacks; Would prefer to take 16 bits from reserved2, and not bump the size of the struct.
Review of attachment 130776 [details] [review]: So in the big picture, I won't say this is wrong but it feels odd. The callback is also a field; but are we setting up any association there? I'd like to merge this patch with the next one since it's really not complete on its own.
Created attachment 146566 [details] [review] addresses comments
(In reply to comment #23) > Review of attachment 130776 [details] [review]: > > So in the big picture, I won't say this is wrong but it feels odd. The > callback is also a field; but are we setting up any association there? I'd > like to merge this patch with the next one since it's really not complete on > its own. Sorry, I'm not familiar enough with this code, can you extend on this? Or catch me in #introspection if you wish.
(In reply to comment #25) > (In reply to comment #23) > > Review of attachment 130776 [details] [review] [details]: > > > > So in the big picture, I won't say this is wrong but it feels odd. The > > callback is also a field; but are we setting up any association there? I'd > > like to merge this patch with the next one since it's really not complete on > > its own. > > Sorry, I'm not familiar enough with this code, can you extend on this? Or catch > me in #introspection if you wish. There's a GFieldInfo for structs accessible with g_struct_info_get_field, but I was wondering whether there's a way to access the callback from that. I'm looking a bit closer at this now.
What I'm getting at is say from foo.h we have now: struct _FooSubInterfaceIface { GTypeInterface parent_iface; /* signals */ void (*destroy_event) (FooSubInterface *self); /* virtual table */ void (*do_bar) (FooSubInterface *self); }; but this just scans as: <record name="SubInterfaceIface" glib:is-gtype-struct="1"> <field name="parent_iface"> <type name="GObject.TypeInterface"/> </field> </record> This was the case before this patch, but the problem is that this means that anyone who's computing offsets manually won't get the right ones. Now the further problem with adding a field here for callbacks is that I don't think we have a way to reference them cleanly from GITypeInfo, because that is only able to point to elements in the global table, not a nested element.
After an IRC discussion we concluded that a nicer way to do this would be to take a bit from SimpleTypeBlob to say "this offset is parent-relative". Then instead of having g_struct_info_get_callback, we can just look for the field, and get the GICallbackInfo from that.
Created attachment 147027 [details] [review] make fields contain callbacks
Created attachment 147073 [details] backtrace of a crasher caused(?) by the patch i get heap corruption running the tests with this patch applied write_callback_info() is in the backtrace so i guess the problem is caused by the patch from tests/scanner: ../../tools/g-ir-generate --includedir=. --includedir=../../gir annotation-1.0.typelib -o annotation-1.0.tgir
it is no longer appropriate for g_type_info_new() to chain up to g_info_new() since you changed the structure to include the 'is_embedded' field. these two structures are now of different size -- you need to allocate a bigger chunk of memory.
Created attachment 147118 [details] [review] fixes corruption heap found by desrt
Review of attachment 147118 [details] [review]: Overall looks great, thanks for going the extra mile on this! I think the result is fairly nice, and leads the way if we need to handle other embedded type cases in the future (anonymous enums come to mind). Just had one small comment, see below. ::: girepository/ginfo.c @@ +934,3 @@ + if (info->is_embedded) + + SimpleTypeBlob *type; Thought; maybe we could put the type tag inside the "type" member of the field? Just to avoid hardcoding embedded = callback. To do this change I think in the girnode.c part where we do blob->type.offset = 0; change to blob->type.offset = GI_INFO_TYPE_CALLBACK; Then in this function instead of passing GI_INFO_TYPE_CALLBACK we can pass type->offset (I believe) ::: girepository/girnode.c @@ +1612,3 @@ + blob->has_embedded_type = TRUE; + { + if (field->callback) Here for writing GI_INFO_TYPE_CALLBACK instead of 0.
Created attachment 147298 [details] [review] Implement callbacks as part of struct fields. Fixes #557383 gir: embed <callback> inside <field> typelib: if a field contains a callback, store it just after the FieldBlob girepository API: no additions
Created attachment 147395 [details] [review] Don't put the callback element inside the field if it's not a glib struct
Pushed the patches from above. Now each vfunc has its corresponding callback in the glib struct class. The offset of the callback is retrieved from the field it's embedded in. I think this one can be closed now.
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]