GNOME Bugzilla – Bug 646635
Fix introspection of GLib
Last modified: 2015-02-07 16:48:44 UTC
The summary is pretty generic, because this bug is intended for a few patches that I have prepared, against glib and gobject-introspection, and that help with introspecting GLib. Some of them are necessary for my implementation of bug 622344.
Created attachment 185045 [details] [review] Add boxed types definition for GLib Use the new glib-boxed.h header from gobject to pair structure definitions with boxed types in the GLib namespace, improving the introspection coverage and removing some hacks. At the same time, add support for get_gtype() functions instead of get_type().
Created attachment 185046 [details] [review] GLib: update source Source was regenerated using g-ir-annotation-tool from GLib master (private branch)
Created attachment 185047 [details] [review] GObject: move GLib boxed definitions to a separate header This way it is possible to pull them into the GLib GIR file. At the same time, add boxed types for GVariantIter and GVariantBuilder.
Created attachment 185048 [details] [review] GVariant: fix introspection annotations Add transfer annotations for most functions, as well as some (array) and (skip) for functions that use varargs.
Created attachment 185049 [details] [review] gbase64: fix introspection annotations Makes the g_base64_* functions usable from introspection GLib bindings (gjs, currently, as both vala and pygobject use manual bindings for GLib)
Created attachment 185050 [details] [review] gfileutils: fix introspection annotations g_file_get_contents returns a binary array, not a string. Expose that in annotations.
Created attachment 186457 [details] [review] gfileutils, gspawn: fix introspection annotations Fix various (out) arguments, (allow-none) and (array zero-terminated=1) for g_get_*_dirs(), g_spawn_*() and some others.
Created attachment 186458 [details] [review] giscanner: read (array) and (element-type) annotations for fields This way fields are no longer limited to basic types, and can be supported without accessor methods.
Created attachment 186459 [details] [review] GScannerParser: recognize character constants Some enumerations (like GVariantClass) use characters instead of plain integers, so we need to recognize them.
Created attachment 186460 [details] [review] Update glib and gio sources. Regenerated using g-ir-annotation-tool
Created attachment 186462 [details] [review] gdbusintrospection: fix introspection of DBus Introspection structures Correctly mark fields as arrays (requires changing gobject-introspection to pick these).
Created attachment 187132 [details] [review] GDBusMethodInvocation: fix introspection annotations In particular (transfer full), to avoid a refcounting bug and a segmentation fault.
Comment on attachment 187132 [details] [review] GDBusMethodInvocation: fix introspection annotations (In reply to comment #12) > Created an attachment (id=187132) [details] [review] > GDBusMethodInvocation: fix introspection annotations > > In particular (transfer full), to avoid a refcounting bug and a > segmentation fault. Looks good to me. Funny enough, this came up in bug 647577 comment 16 so glad to see it fixed. Thanks.
(In reply to comment #11) > Created an attachment (id=186462) [details] [review] > gdbusintrospection: fix introspection of DBus Introspection structures > > Correctly mark fields as arrays (requires changing gobject-introspection > to pick these). So you are suggesting - * @annotations: A pointer to a %NULL-terminated array of pointers to #GDBusAnnotationInfo structures or %NULL if there are no annotations. + * @annotations: (array zero-terminated=1): A pointer to a %NULL-terminated array of pointers to #GDBusAnnotationInfo structures or %NULL if there are no annotations. for a C struct like this typedef struct { .. GDBusAnnotationInfo **annotations; .. } (see http://developer.gnome.org/gio/unstable/gio-D-Bus-Introspection-Data.html#GDBusNodeInfo for one of the C structs in question) which I'm not sure is correct.... I would think that (array zero-terminated=1) would only work if the annotations member was an array of elements, e.g. typedef struct { .. GDBusAnnotationInfo *annotations; .. } and not an array of _pointers_ to elements like what we have here (This is basically GPtrArray vs GArray all over). Additionally, shouldn't you also allow (allow-none) since the struct member itself can be NULL? FWIW, last time I asked about this on the #introspection IRC channel, I was told that introspection didn't support struct member access and such specialized C arrays.
Comment on attachment 187132 [details] [review] GDBusMethodInvocation: fix introspection annotations Attachment 187132 [details] pushed as 53dc341 - GDBusMethodInvocation: fix introspection annotations
(In reply to comment #14) > (In reply to comment #11) > > Created an attachment (id=186462) [details] [review] [details] [review] > > gdbusintrospection: fix introspection of DBus Introspection structures > > > > Correctly mark fields as arrays (requires changing gobject-introspection > > to pick these). > > So you are suggesting > > - * @annotations: A pointer to a %NULL-terminated array of pointers to > #GDBusAnnotationInfo structures or %NULL if there are no annotations. > + * @annotations: (array zero-terminated=1): A pointer to a %NULL-terminated > array of pointers to #GDBusAnnotationInfo structures or %NULL if there are no > annotations. > > for a C struct like this > > typedef struct { > .. > GDBusAnnotationInfo **annotations; > .. > } > > (see > http://developer.gnome.org/gio/unstable/gio-D-Bus-Introspection-Data.html#GDBusNodeInfo > for one of the C structs in question) > > which I'm not sure is correct.... I would think that (array zero-terminated=1) > would only work if the annotations member was an array of elements, e.g. > > typedef struct { > .. > GDBusAnnotationInfo *annotations; > .. > } > > and not an array of _pointers_ to elements like what we have here (This is > basically GPtrArray vs GArray all over). Additionally, shouldn't you also > allow (allow-none) since the struct member itself can be NULL? (array), on an element of type NSType*, is a short-hand for "(array) (element-type NS.Type)", so there it is a short-hand for "(array) (element-type Gio.DBusAnnotationInfo)". In addition, all object and boxed types are always passed by pointer, so there is no way to express an array of GDBusAnnotationInfo structures. (gobject-introspection records a "pointer indirection level" when scanning, but it is not serialized in the GIR) > > FWIW, last time I asked about this on the #introspection IRC channel, I was > told that introspection didn't support struct member access and such > specialized C arrays. g_struct_info_get_field supports everything which can be represented as a GArgument. The problem is setting the field, but if you avoid it, nothing bad happens. (It is treated as a (transfer none) function, and the result is copied/reffed when wrapping).
Review of attachment 185049 [details] [review]: Looks fine.
Review of attachment 186459 [details] [review]: I'd prefer taking this patch with a regression test, but the code looks fine.
Review of attachment 185048 [details] [review]: This looks generally OK, but honestly I think it's far saner to implement GVariant bindings in C.
Review of attachment 186457 [details] [review]: ::: glib/gfileutils.c @@ +807,3 @@ * g_file_get_contents: * @filename: name of a file to read contents from, in the GLib file name encoding + * @contents: (out): location to store an allocated string, use g_free() to free Note actually this is in actuality a byte array as discussed from https://bugzilla.gnome.org/show_bug.cgi?id=646333 I'll file a new gnome-shell change to port the g_file_get_contents users over so we can fix this annotation. ::: glib/gvariant-core.c @@ +849,3 @@ gsize index_) { + g_return_val_if_fail (g_variant_n_children (value) > index_, NULL); This isn't an annotation change - split to a separate patch?
Review of attachment 186457 [details] [review]: Two comments:
Review of attachment 186458 [details] [review]: I'm not a big fan of encouraging direct field access for complex types; also, were we to do this, we'd need regression tests.
Created attachment 187649 [details] [review] GScannerParser: recognize character constants Some enumerations (like GVariantClass) use characters instead of plain integers, so we need to recognize them. Updated with scanner tests.
Comment on attachment 185049 [details] [review] gbase64: fix introspection annotations Attachment 185049 [details] pushed as eebb364 - gbase64: fix introspection annotations
(In reply to comment #22) > Review of attachment 186458 [details] [review]: > > I'm not a big fan of encouraging direct field access for complex types; also, > were we to do this, we'd need regression tests. Note that otherwise nothing stops a programmer from accessing the field, and making the whole program segfault (because it access CType** as it was CType*). (In reply to comment #20) > Review of attachment 186457 [details] [review]: > > ::: glib/gfileutils.c > @@ +807,3 @@ > * g_file_get_contents: > * @filename: name of a file to read contents from, in the GLib file name > encoding > + * @contents: (out): location to store an allocated string, use g_free() to > free > > Note actually this is in actuality a byte array as discussed from > https://bugzilla.gnome.org/show_bug.cgi?id=646333 > > I'll file a new gnome-shell change to port the g_file_get_contents users over > so we can fix this annotation. Yeah, the annotation was fixed in the handwritten glib-2.0.c, but I regenerated it with g-ir-annotation-tool, so needed that fix to run the shell. > > ::: glib/gvariant-core.c > @@ +849,3 @@ > gsize index_) > { > + g_return_val_if_fail (g_variant_n_children (value) > index_, NULL); > > This isn't an annotation change - split to a separate patch? Ops. Yes, sorry.
Review of attachment 187649 [details] [review]: Looks good, thanks.
Review of attachment 185047 [details] [review]: The boxed types for GVariant should be a separate patch. So, I see what you're trying to do here, and it does make sense - but the problem is that we still have the boxed types inside GObject-2.0.gir. I think to be truly correct this patch should also ensure that they *aren't* scanned into there.
Created attachment 187660 [details] [review] giscanner: read (array) and (element-type) annotations for fields This way fields are no longer limited to basic types, and can be supported without accessor methods. Updated with regression tests.
Review of attachment 187660 [details] [review]: Thanks for adding tests.
(In reply to comment #19) > Review of attachment 185048 [details] [review]: > > This looks generally OK, but honestly I think it's far saner to implement > GVariant bindings in C. Why? GVariant is a native GObject type, like GBoxed, GObject or GParamSpec, and it should be supported in the same way, accessing introspected methods.
Created attachment 187726 [details] [review] GObject: move GLib boxed definitions to a separate header This way it is possible to pull them into the GLib GIR file. The boxed types are not duplicated if the other patch to gobject-introspection is applied at the same time.
Created attachment 187727 [details] [review] Add boxed types for GVariantIter and GVariantBuilder Add boxed type definition, so they can used from introspection bindings.
(In reply to comment #30) > (In reply to comment #19) > > Review of attachment 185048 [details] [review] [details]: > > > > This looks generally OK, but honestly I think it's far saner to implement > > GVariant bindings in C. > > Why? GVariant is a native GObject type, like GBoxed, There is no "GBoxed" > GObject All of GObject is manually bound in both gjs and pygobject. > or GParamSpec, Sort of an ugly case; I know people wanted it but... > and > it should be supported in the same way, accessing introspected methods. But again, there is no reason to have any API other than: GVariant.unpack() GVariant.get_signature() new GVariant({ signature: 's', value: 'foo' }); So implementing it in JavaScript is just pure overhead.
Created attachment 187734 [details] [review] Consolidate GLib types Include GIOCondition and GIOChannel into glib-types.h, so they're correctly associated with GLib.
Review of attachment 187734 [details] [review]: ::: gobject/gobject.symbols @@ +42,3 @@ g_regex_get_type G_GNUC_CONST +g_variant_iter_get_type G_GNUC_CONST +g_variant_builder_get_type G_GNUC_CONST Should be in the other patch.
Created attachment 187742 [details] [review] Add boxed types definition for GLib Use the new glib-boxed.h header from gobject to pair structure definitions with boxed types in the GLib namespace, improving the introspection coverage and removing some hacks. At the same time, add support for get_gtype() functions instead of get_type(). Updated for glib-boxed.h -> glib-types.h
Can you squash together the patches for glib into one? It's a bit hard to follow as is now.
Created attachment 187838 [details] [review] GObject: move GLib type definitions to a separate header This way it is possible to pull them into the GLib GIR file.
Created attachment 187839 [details] [review] Add boxed types for GVariantIter and GVariantBuilder Add boxed type definition, so they can used from introspection bindings.
(In reply to comment #33) > (In reply to comment #30) > > (In reply to comment #19) > > > Review of attachment 185048 [details] [review] [details] [details]: > > > > > > This looks generally OK, but honestly I think it's far saner to implement > > > GVariant bindings in C. > > > > Why? GVariant is a native GObject type, like GBoxed, > > There is no "GBoxed" G_TYPE_BOXED is GBoxed. > > GObject > > All of GObject is manually bound in both gjs and pygobject. False, at least in gjs: only g_signal_connect / g_signal_disconnect are manually bound, the rest (run_dispose, notify, notify_by_pspec, freeze_notify, thaw_notify, {get,set}_property, *_data, watch_closure, even ref/unref) comes from GObject Intrsopection. > > or GParamSpec, > > Sort of an ugly case; I know people wanted it but... > > > and > > it should be supported in the same way, accessing introspected methods. > > But again, there is no reason to have any API other than: > > GVariant.unpack() > GVariant.get_signature() > new GVariant({ signature: 's', value: 'foo' }); > > So implementing it in JavaScript is just pure overhead. Well, GVariant.get_child_value is useful as well, and then all the GVariant.new_*, and probably GVariant.classify, and print/print_string, and parse, and whatever GLib 2.32, 2.34, 2.36... will add. So you still have to make it expose GIRepositoryFunction, at which point implementing it in Javascript is a lot easier to write, and avoids the cumbersome JSAPI (with the rooting problems, the jsval/JSObject/JSFunction/JSString mess, the horrible gjs macros...)
(In reply to comment #40) > > G_TYPE_BOXED is GBoxed. Hmm...okay. > False, at least in gjs: only g_signal_connect / g_signal_disconnect are > manually bound, the rest (run_dispose, notify, notify_by_pspec, freeze_notify, > thaw_notify, {get,set}_property, *_data, watch_closure, even ref/unref) comes > from GObject Intrsopection. Okay, you're right, we're leaking some of this stuff through currently. But about half of it (like *_data) don't work because they expose raw pointers, or are varargs, etc. So I'd still argue the right fix is to bind manually the things we care about, which might be "run_dispose" and that's it for now. Some of the other bits (property notify specifically) would be part of a story about subclassing GObject in JS. > Well, GVariant.get_child_value is useful as well, Is it? > and then all the > GVariant.new_*, Not clear to me why those are better than new GVariant("s", 'some string') etc. > and probably GVariant.classify, and print/print_string, and > parse, print/parse I agree are useful. > So you still have to make it expose GIRepositoryFunction, at which point > implementing it in Javascript is a lot easier to write, and avoids the > cumbersome JSAPI (with the rooting problems, the > jsval/JSObject/JSFunction/JSString mess, the horrible gjs macros...) Well...I'm going to make that a lot simpler soon by axing off support for pre-2.0 spidermonkey, but yes. What I really care about though is the API exposed to JS authors; the implementation matters less to me. Basically I don't want people doing GVariant.new_string() when new GVariant("s", 'some string') works perfectly well.
Review of attachment 187838 [details] [review]: ::: gobject/Makefile.am @@ +294,3 @@ $(CPP) -P - <$(top_srcdir)/build/win32/vs10/gobject.vcxprojin >$@ rm libgobject.vs10.sourcefiles + Spurious whitespace change? @@ +304,3 @@ $(CPP) -P - <$(top_srcdir)/build/win32/vs10/gobject.vcxproj.filtersin >$@ rm libgobject.vs10.sourcefiles.filters + Ditto. ::: gobject/glib-types.h @@ +1,2 @@ +/* GObject - GLib Type, Object, Parameter and Signal Library + * Copyright (C) 2000-2001 Red Hat, Inc. Since you made this new file, you might as well add your name as a copyright holder; the changes are nontrivial enough. Up to you of course. @@ +27,3 @@ +typedef gsize GType; +#endif + Hmm, ugly. Can we do this inside g-ir-scanner? Why is this needed? @@ +28,3 @@ +#endif + +#if !defined(GOBJECT_COMPILATION) || !defined(GIR_COMPILATION) Wouldn't this be less confusing written as: #if defined(GOBJECT_COMPILATION) || defined(GIR_COMPILATION) Except, note that Makefile-gir.am in g-i does -DGOBJECT_COMPILATION So what is this conditional accomplishing?
Review of attachment 187839 [details] [review]: Looks good pending the previous commit landing.
Review of attachment 187742 [details] [review]: ::: Makefile-gir.am @@ +63,3 @@ endif +GLib_2_0_gir_LIBS = $(GLIB_LIBRARY) $(GOBJECT_LIBRARY) Honestly this makes me wonder whether we should just bite the bullet and fold GObject.gir into GLib.gir - vala does this. @@ +77,3 @@ -DGETTEXT_PACKAGE=Dummy \ -DGLIB_COMPILATION \ + -DGIR_COMPILATION \ Note that we already #define __GI_SCANNER__ automatically. See giscanner/sourcescanner.py:275. Can we just reuse that? Note if doing so you'd need to fix your GLib patch too. @@ +116,3 @@ GObject_2_0_gir_CFLAGS = \ -DGOBJECT_COMPILATION \ + -DGIR_COMPILATION \ Ditto.
(In reply to comment #44) > Review of attachment 187742 [details] [review]: > > ::: Makefile-gir.am > @@ +63,3 @@ > endif > > +GLib_2_0_gir_LIBS = $(GLIB_LIBRARY) $(GOBJECT_LIBRARY) > > Honestly this makes me wonder whether we should just bite the bullet and fold > GObject.gir into GLib.gir - vala does this. Some debate about this on #introspection, and a decision basically against for now.
Comment on attachment 187838 [details] [review] GObject: move GLib type definitions to a separate header Attachment 187838 [details] pushed as 7c63370 - GObject: move GLib type definitions to a separate header
boxing GVariantBuilder (and particularly) GVariantIter seem wrong to me. both of them are supposed to be used locally and for rather short periods of time.
(In reply to comment #47) > boxing GVariantBuilder (and particularly) GVariantIter seem wrong to me. > > both of them are supposed to be used locally and for rather short periods of > time. Giovanni, do we need these boxed types for the overrides? From a quick scan of the source code it looks like no?
(In reply to comment #48) > (In reply to comment #47) > > boxing GVariantBuilder (and particularly) GVariantIter seem wrong to me. > > > > both of them are supposed to be used locally and for rather short periods of > > time. > > Giovanni, do we need these boxed types for the overrides? From a quick scan of > the source code it looks like no? GVariantIter is not needed, whereas GVariantBuilder is, for 'a*' variants.
(In reply to comment #49) > (In reply to comment #48) > > (In reply to comment #47) > > > boxing GVariantBuilder (and particularly) GVariantIter seem wrong to me. > > > > > > both of them are supposed to be used locally and for rather short periods of > > > time. > > > > Giovanni, do we need these boxed types for the overrides? From a quick scan of > > the source code it looks like no? > > GVariantIter is not needed, whereas GVariantBuilder is, for 'a*' variants. I see. Ok, I can definitely see the argument for GVariantBuilder. desrt, any objections?
The only non-GVariantBuilder API that takes a GVariantBuilder is g_variant_new() and its convenience wrappers. Those are varargs functions and meant solely to be used from C. GVariantBuilder and GVariantIter are really only meant to be C convenience API. You can use g_variant_new_tuple, g_variant_new_array and g_variant_new_dict_entry to construct anything you need to and you'll probably end up with higher performance that way too. Certainly, programmers should not be using GVariantBuilder/Iter from languages with facilities for 'more natural' construction of arrays/tuples/dictionaries.
(In reply to comment #51) > The only non-GVariantBuilder API that takes a GVariantBuilder is > g_variant_new() and its convenience wrappers. Those are varargs functions and > meant solely to be used from C. > > GVariantBuilder and GVariantIter are really only meant to be C convenience API. > > You can use g_variant_new_tuple, g_variant_new_array and > g_variant_new_dict_entry to construct anything you need to and you'll probably > end up with higher performance that way too. > > Certainly, programmers should not be using GVariantBuilder/Iter from languages > with facilities for 'more natural' construction of arrays/tuples/dictionaries. I didn't notice g_variant_new_array. In that case, GVariantBuilder is not needed, and if you want you can leave it out of introspection. (But in that case (skip) it)
Review of attachment 186462 [details] [review]: Looks correct to me.
Review of attachment 187839 [details] [review]: I agree
(In reply to comment #54) > Review of attachment 187839 [details] [review]: > > I agree Wait, agree with whom? I though you all agreed that GVariantIter/GVariantBuilder were to be left out of introspection. What is this sudden change of mind?
+#if !defined(G_DISABLE_DEPRECATED) || defined(__GI_SCANNER__) +GType g_variant_get_gtype (void) G_GNUC_CONST; +#endif Why export this to the scanner? GVariant is now fundamental; you shouldn't use the deprecated g_variant_get_gtype to get its type, just use G_TYPE_VARIANT directly like for the other fundamentals too, like G_TYPE_INT etc.
(In reply to comment #56) > +#if !defined(G_DISABLE_DEPRECATED) || defined(__GI_SCANNER__) > +GType g_variant_get_gtype (void) G_GNUC_CONST; > +#endif > > Why export this to the scanner? GVariant is now fundamental; you shouldn't use > the deprecated g_variant_get_gtype to get its type, just use G_TYPE_VARIANT > directly like for the other fundamentals too, like G_TYPE_INT etc. The problem is that I need g_registered_type_info_get_g_type to return something useful, which can happen only if gobject-introspection scans a function returning it. (Or I would need to reintroduce all the sort of hacks giscanner currently has for GObject and GInitiallyUnowned)
Comment on attachment 186462 [details] [review] gdbusintrospection: fix introspection of DBus Introspection structures Attachment 186462 [details] pushed as 0aae977 - gdbusintrospection: fix introspection of DBus Introspection structures
Created attachment 188617 [details] [review] Rework how fundamental GObject types are introspected A solution to the problem of using g_variant_get_g_type (even if it will be there until GLib 4.0, so not really an immediate issue). ------ Change the special code for handling GObject and GInitiallyUnowned so that it exposes GParamSpec as a class, and it allows GVariant to have a GType without using the deprecate g_variant_get_gtype. It is a sort of ABI break, in that new typelibs won't work with previous versions of libgirepository.
Created attachment 188627 [details] [review] Rework how fundamental GObject types are introspected Change the special code for handling GObject and GInitiallyUnowned so that it exposes GParamSpec as a class, and it allows GVariant to have a GType without using the deprecate g_variant_get_gtype. It is a sort of ABI break, in that new typelibs won't work with previous versions of libgirepository. Fixed a bug that prevented constructors from being associated to GVariant (by failing to set the c_simbol_prefix)
Review of attachment 188627 [details] [review]: This is very tricky code, and it's hard to review. I'd like a little bit better example of why we need this change. We're trying to allow binding GParamSpec; ok, right now it shows up as an unmanaged <record>. But why do we need to bind GParamSpec for GVariant?
Review of attachment 185048 [details] [review]: One bug. ::: glib/gvariant.c @@ +694,3 @@ * g_variant_get_variant: * @value: a variant #GVariant instance + * @returns: (transfer none): the item contained in the variant Why isn't this one (transfer full)? You used (transfer none) for g_variant_get_maybe() which also just internally calls g_variant_get_child_value(). Pretty sure this one is incorrect. @@ +2657,3 @@ /** * g_variant_iter_free: + * @iter: (transfer full): a heap-allocated #GVariantIter We don't in practice really support transfer for input parameters, but this is harmless.
Comment on attachment 185048 [details] [review] GVariant: fix introspection annotations Attachment 185048 [details] pushed as e61fa51 - GVariant: fix introspection annotations
(In reply to comment #63) > (From update of attachment 185048 [details] [review]) > Attachment 185048 [details] pushed as e61fa51 - GVariant: fix introspection annotations I pushed with that fix for the transfer.
Comment on attachment 186457 [details] [review] gfileutils, gspawn: fix introspection annotations This one is not entirely committed; I skipped the g_spawn_*() ones because the argument vectors aren't utf-8. What we really need is a Gio.Process class.
Comment on attachment 187839 [details] [review] Add boxed types for GVariantIter and GVariantBuilder Pushed just the GVariantBuilder boxed type per discussion.
Created attachment 189006 [details] [review] gutils, gspawn: fix introspection annotations Fix various (out) arguments, (allow-none) and (array zero-terminated=1) for g_spawn_*() and some others. ----- The missing parts, to ensure they're not forgotten. (gnome-shell will fail to start once glib sources are regenerated with g-ir-annotation-tool if these annotations are not added)
Created attachment 189007 [details] [review] gatomic: ensure that GCC atomic macros are not scanned Protect GCC atomic macros with #ifdefs, as g-ir-scanner is unable to parse __extension__ and inline statements.
Created attachment 189008 [details] [review] gvariant: fix introspection annotations g_variant_get_strv and g_variant_get_bytestring return arrays that are null terminated and have an explicit length. Since gjs doesn't support (out) arrays with length, mark them also null-terminated (but leave the length annotation, so pygobject can remove the argument)
Created attachment 189009 [details] [review] gtype: hide internal functions from the scanner gobject-introspection cannot recognize that functions are G_GNUC_INTERNAL, so it will consider them public, meaning that dynamic bindings will fail to retrieve the symbol. This resulted in gjs failing to define GObject class, because it was trying to add g_object_type_init as a static method. (Actually, gjs fails to define GInitiallyUnowned now, because GObject static methods live there, but that's a different bug, in gobject-introspection)
Created attachment 189013 [details] [review] Always add a zero-terminated attribute when it cannot be implied g-ir-compiler assumes that an array is zero terminated when the attribute is absent and there is no other attribute (length and fixed-size), but g-ir-scanner only added the attribute when it is 0. This means that an explicit zero-terminated=1 annotation would have had no effect. Fix that and at the same time ensure that all other arrays are not zero-terminated by default.
(In reply to comment #61) > Review of attachment 188627 [details] [review]: > > This is very tricky code, and it's hard to review. I'd like a little bit > better example of why we need this change. > > We're trying to allow binding GParamSpec; ok, right now it shows up as an > unmanaged <record>. But why do we need to bind GParamSpec for GVariant? That patch does three things. One is untying get_type=intern from G_TYPE_OBJECT, by using the type_name in gi_registered_type_info_get_g_type. The second is ensuring that fundamental GObject types (GObject, GParamSpec, GVariant) don't have their get_type function called, to avoid bogus results. The third is ensuring that "GObject.Object" is the parent of all GObject-derived types, and that parent is set (or left as-is) correctly for fundamentals, for GParamSpec/GVariant and for GInterface. (Now GObject.Object has parent=GObject.Object). While I was there, I though I could just generate a good ast.Node for GParamSpec and fix c_symbol_prefix for GInitiallyUnowned, but it wasn't my primary objective.
Created attachment 189018 [details] [review] Add boxed types definition for GLib Use the new glib-boxed.h header from gobject to pair structure definitions with boxed types in the GLib namespace, improving the introspection coverage and removing some hacks. At the same time, add support for get_gtype() functions instead of get_type().
Review of attachment 189007 [details] [review]: Hmm...so what was bothering me is why we weren't blowing up before. There were other uses of __extension__ in the glib headers. I'll look into this.
Comment on attachment 189007 [details] [review] gatomic: ensure that GCC atomic macros are not scanned Not needed now that __extension__({}) is parsed.
Comment on attachment 189009 [details] [review] gtype: hide internal functions from the scanner See bug 651745 which I'd prefer instead of one-off hacks like this.
Comment on attachment 189009 [details] [review] gtype: hide internal functions from the scanner Certainly. This patch is now obsolete.
Comment on attachment 189008 [details] [review] gvariant: fix introspection annotations Attachment 189008 [details] pushed as f2bd54d - gvariant: fix introspection annotations
Review of attachment 189013 [details] [review]: Oh, nice catch. Ideally, we would change g-ir-compiler to not assume zero-termination, but that may be too backwards incompatible.
Review of attachment 189013 [details] [review]: However, this patch mixes changes for the GVariant transfers.
Review of attachment 189018 [details] [review]: This looks good, though I'm going to split out the _get_gtype() change and add a test case for it.
Review of attachment 189018 [details] [review]: ::: Makefile-gir.am @@ -66,2 @@ GLib_2_0_gir_SCANNERFLAGS = \ - --external-library \ Why are we removing this? @@ -70,3 @@ --symbol-prefix=g \ --symbol-prefix=glib \ - --c-include="glib.h" \ This change looks plain wrong...we're no longer passing the $(GLib_2_0_gir_DOCSRC) to g-ir-scanner.
Created attachment 189189 [details] [review] Add boxed types definition for GLib Some fixes from Colin Walters <walters@verbum.org>
Created attachment 189190 [details] [review] Add boxed types definition for GLib Re-add missing \
Thanks for all the work here Giovanni! I'm trying really hard to get this all in, hoping to do some more pushing this weekend.
Created attachment 189218 [details] [review] Add boxed types definition for GLib Use the new glib-boxed.h header from gobject to pair structure definitions with boxed types in the GLib namespace, improving the introspection coverage and removing some hacks. Some fixes from Colin Walters <walters@verbum.org> A less hackish version.
Created attachment 189219 [details] [review] Always add a zero-terminated attribute when it cannot be implied g-ir-compiler assumes that an array is zero terminated when the attribute is absent and there is no other attribute (length and fixed-size), but g-ir-scanner only added the attribute when it is 0. This means that an explicit zero-terminated=1 annotation would have had no effect. Fix that and at the same time ensure that all other arrays are not zero-terminated by default. Changes for GVariant transfer removed by automerging.
Review of attachment 189218 [details] [review]: It looks like this one is missing my code to strip duplication from GObject: + # Strip out things that are duplicated between GLib/GObject + if self._namespace.name == 'GObject': + to_delete = [] + glib = self._transformer._includes['GLib'] + for glibnode in glib.itervalues(): + gobjnode = self._namespace.get(glibnode.name) + if gobjnode is not None: + self._namespace.remove(gobjnode)
Review of attachment 189218 [details] [review]: Oh nevermind, I see you did it by filtering out glib-types.h, OK.
Comment on attachment 189218 [details] [review] Add boxed types definition for GLib Attachment 189218 [details] pushed as 7c93dc9 - Add boxed types definition for GLib
Comment on attachment 189219 [details] [review] Always add a zero-terminated attribute when it cannot be implied Attachment 189219 [details] pushed as 3e0292c - Always add a zero-terminated attribute when it cannot be implied
Created attachment 189276 [details] [review] Fix accessing structure fields that are arrays We need to distinguish inline arrays inside structures, and arrays that are pointers and annotations, and we can do it with g_type_info_is_pointer(), setting it to FALSE for fixed size arrays. As a side effect, (array fixed-size=N) on a pointer type has no longer the expected result.
Created attachment 189277 [details] [review] gutils, gspawn: fix introspection annotations Fix various (out) arguments, (allow-none) and (array zero-terminated=1) for g_spawn_*() and some others.
Review of attachment 189276 [details] [review]: This code has seen patches from 3 different people now, but this looks like a much cleaner fix =)
Review of attachment 188627 [details] [review]: Ok; I think this is fine.
Review of attachment 189277 [details] [review]: ::: glib/gspawn.c @@ +86,3 @@ * g_spawn_async: + * @working_directory: (allow-none): child's current working directory, or %NULL to inherit parent's + * @argv: (array zero-terminated=1): child's argument vector I think I mentioned this before but the argv is actually bytestrings, but I don't think we have a way to express that =( (element-type bytestring) maybe? We'd need it in introspection. We have the same problem for Gtk.init() too. @@ +90,2 @@ * @flags: flags from #GSpawnFlags + * @child_setup: (scope async) (allow-none): function to run in the child just before exec() Note it's pretty dangerous to use this from bindings; for example with Spidermonkey, the garbage collection thread will be killed by fork(). And if it was presently holding a lock, it will never be unlocked. We could use (type gpointer) to force it to be un-introspectable, and have a different function...but ultimately I want to make a GProcess class, so let's not worry about it for now. @@ +211,3 @@ + * @child_setup: (scope async) (allow-none): function to run in the child just before exec() + * @user_data: (closure): user data for @child_setup + * @standard_output: (out) (allow-none): return location for child output, or %NULL And these are also (array) (element-type uint8) @@ +212,3 @@ + * @user_data: (closure): user data for @child_setup + * @standard_output: (out) (allow-none): return location for child output, or %NULL + * @standard_error: (out) (allow-none): return location for child error messages, or %NULL Ditto.
(In reply to comment #96) > + * @argv: (array zero-terminated=1): child's argument vector > > I think I mentioned this before but the argv is actually bytestrings, but I > don't think we have a way to express that =( A bytestring is just an array of uint8 isn't it? But argvs are specifically "filename" encoding anyway, and we do have an annotation for that, don't we?
(In reply to comment #97) > (In reply to comment #96) > > + * @argv: (array zero-terminated=1): child's argument vector > > > > I think I mentioned this before but the argv is actually bytestrings, but I > > don't think we have a way to express that =( > > A bytestring is just an array of uint8 isn't it? > > But argvs are specifically "filename" encoding anyway, and we do have an > annotation for that, don't we? Hmm, good point. You know honestly it's tempting to just rename GI_TYPE_TAG_FILENAME to GI_TYPE_TAG_BYTESTRING and (type filename) to (type bytestring). (With compatibility #defines etc.)
So actually, i was wrong; argv arguments are either locale encoding or filename encoding, depending on whether or not they're filenames. And in theory, locale encoding and filename encoding could each be a different non-utf8 encoding. Yuck. I bet no one gets this right ever. Calling them bytestrings doesn't really help though unless we expose g_locale_{to,from}_utf8 and g_filename_{to,from}_utf8.
Comment on attachment 189277 [details] [review] gutils, gspawn: fix introspection annotations Attachment 189277 [details] pushed as 37c9775 - gutils, gspawn: fix introspection annotations
Attachment 188627 [details] pushed as 0d7b707 - Rework how fundamental GObject types are introspected Attachment 189276 [details] pushed as aea1a55 - Fix accessing structure fields that are arrays Before this bug can be marked fixed I need to update source files in gobject-introspection. I have regenerated them with g-ir-annotation-tool (the python helpers are no good because they read header files, which is not needed and sometimes wrong); I cannot attach the patch because it is too big for bugzilla, so tell me if it is ok to push.
(In reply to comment #101) > > Before this bug can be marked fixed I need to update source files in > gobject-introspection. I have regenerated them with g-ir-annotation-tool (the > python helpers are no good because they read header files, which is not needed > and sometimes wrong); I cannot attach the patch because it is too big for > bugzilla, so tell me if it is ok to push. Should be done now. I wrote new tools, and also switched to reading annotations for glib-2.0.c
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]