GNOME Bugzilla – Bug 625383
Add missing GI annotations
Last modified: 2011-08-23 23:57:24 UTC
Some API bits in GIO lack basic, non-default gobject-introspection annotations. One of the goals of the introspection team at GUADEC2010 is to advance on this, so expect an avalanche of patch attachments during the next days, fixing annotations here and there.
Created attachment 166656 [details] [review] first set of patches Attached a tar.gz bundle with the first delivery. Feedback is very appreciated at this stage to know I won't be repeating same errors in each patch. cheers
+ * @effective_address: (out): location to store the address that was bound to, or %NULL. Also need (allow-none) + * @source_object: (out) (transfer none): location where #GObject pointer will be stored, or %NULL Also need (allow-none) + * @source_object: (out) (transfer none): location where #GObject pointer will be stored, or %NULL Also need (allow-none) Could you please push them? For the upcoming patches, could you group them in bigger patches? It will make it easier to review. Thanks!
also, please never attach patches as .tar.gz. it prevents us using the review system.
I have just pushed the patches squashed into a single commit, after fixing the issues Tomeu pointed out (I completely missed the (allow-none)s!) From now on, I will always combine annotations into a single patch. Thank you guys for your feedback.
Thanks for the patch. Closing this bug now, since you pushed it.
Lots of annotations are still missing. I just pushed the first set for reviewing/feedback purposes. I'm almost finished with another patch with many more annotations. Hence reopening. BTW, now realized that I forgot to reference the bug number in the commit message, sorry.
Created attachment 166823 [details] [review] patch adding more annotations
Review of attachment 166823 [details] [review]: ::: gio/giomodule.c @@ +408,2 @@ * unload them (enabling on-demand loading) you must call * g_type_module_unuse() on all the modules. Free the list Does this need some transfer annotation ? @@ +744,3 @@ * + * Returns: (transfer none): the #GIOExtension for @extension_point that + * has the given name, or %NULL if there is no extension with that name. allow-none ? ::: gio/gresolver.c @@ +243,3 @@ + * or %NULL on error. You must unref each of the addresses and free the + * list when you are done with it. (You can use g_resolver_free_addresses() + * to do this.) allow-none and transfer full ? @@ +344,3 @@ * + * Return value: (element-type GInetAddress): a #GList of #GInetAddress, + * or %NULL on error. See g_resolver_lookup_by_name() for more details. allow-none ? @@ +549,3 @@ + * Return value: (element-type GSrvTarget): a #GList of #GSrvTarget, + * or %NULL on error. You must free each of the targets and the list when + * you are done with it. allow-none ? @@ +637,3 @@ * + * Return value: (element-type GSrvTarget): a #GList of #GSrvTarget, + * or %NULL on error. See g_resolver_lookup_service() for more details. allow-none ?
(In reply to comment #8) > Review of attachment 166823 [details] [review]: > > ::: gio/giomodule.c > @@ +408,2 @@ > * unload them (enabling on-demand loading) you must call > * g_type_module_unuse() on all the modules. Free the list > > Does this need some transfer annotation ? My understanding is that it doesn't require transfer annotation since the default (transfer-full) is correct in this case (the GIOModules added to the GList, are created everytime and not cached anywhere internally, so the owner has to own them and the list as well). > > @@ +744,3 @@ > * > + * Returns: (transfer none): the #GIOExtension for @extension_point that > + * has the given name, or %NULL if there is no extension with that name. > > allow-none ? > > ::: gio/gresolver.c > @@ +243,3 @@ > + * or %NULL on error. You must unref each of the addresses and free the > + * list when you are done with it. (You can use g_resolver_free_addresses() > + * to do this.) > > allow-none and transfer full ? > > [...] > > @@ +637,3 @@ > * > + * Return value: (element-type GSrvTarget): a #GList of #GSrvTarget, > + * or %NULL on error. See g_resolver_lookup_service() for more details. > > allow-none ? 'allow-none' is a per function parameter annotation, not for return values, AFAIK. I only added explicit annotations where default annotations are incorrect or ambiguous. That's why there is no 'transfer-full' annotations.
Anybody against committing this bug? perhaps I should wait people to return from holidays for more review.
Hi, Sorry for taking so long to get to this bug. The main issue right now is that annotations in GLib itself aren't actually effective, because gobject-introspection only scans the headers. We have 3 possible routes, of which we just need to pick one. I'd actually prefer 3, but will start work on 1. 1) Write a script which extracts the glib headers and doc comments from C source, and cache this in gobject-introspection git. Run the scanner on these cached files. + No impact on glib - Probably several megabytes of cached source in g-i git 2) Move scanning into GLib, but cache the .gir files in git + Starts paving the way for merging g-i in glib - Sort of unfortunate to do until .gir format is totally frozen 3) Move scanning into GLib, don't cache .gir files - If pure bootstrapping, would require building glib, building g-i, building glib again * But no one does this, so it doesn't actually matter.
(In reply to comment #11) > The main issue right now is that > annotations in GLib itself aren't actually effective, because > gobject-introspection only scans the headers. I'm aware of this. My idea is adding annotations in GLib/GIO first, and only after they are in place, add the corresponding annotations in G-I. I don't remember who exactly told me that only annotations that were already in GLib, would be accepted in G-I. > > We have 3 possible routes, of which we just need to pick one. I'd actually > prefer 3, but will start work on 1. > > 1) Write a script which extracts the glib headers and doc comments from C > source, and cache this in gobject-introspection git. Run the scanner on these > cached files. > + No impact on glib > - Probably several megabytes of cached source in g-i git > Yes, this seems like the easiest way for the short term. However, annotations will have to be in GLib anyway for this to work. > 2) Move scanning into GLib, but cache the .gir files in git > + Starts paving the way for merging g-i in glib > - Sort of unfortunate to do until .gir format is totally frozen > > 3) Move scanning into GLib, don't cache .gir files > - If pure bootstrapping, would require building glib, building g-i, building > glib again > * But no one does this, so it doesn't actually matter. In fresh environments I suspect 2) would require building GLib twice as well. Is there any agreed solution for the double-building issue? Is there a roadmap to 3)? Well, please let me know if you need help with any of these plans. Regarding this patch, I think I should review and update it for latests changes on G-I default annotations policy. Does it makes sense to try push these annotations in G-I still? or should we just focus on 1), 2) or 3)? cheers
g_input_stream_read() or anything related to read, in ginputstream.c, don't have (out caller-allocates) like annotations. How are we supposed to use the python binding without out buffer? >>> stream 14: <__main__.GLocalFileInputStream object at 0x853f194 (GLocalFileInputStream at 0x858c200)> >>> stream.read(10, None) Traceback (most recent call last):
+ Trace 227404
stream.read(10, None)
return info.invoke(*args)
Most of the annotations from this patch had since been added by other people, but I committed the remaining ones.