After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 625383 - Add missing GI annotations
Add missing GI annotations
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2010-07-27 10:04 UTC by Eduardo Lima Mitev
Modified: 2011-08-23 23:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
first set of patches (2.08 KB, patch)
2010-07-27 15:46 UTC, Eduardo Lima Mitev
committed Details | Review
patch adding more annotations (6.83 KB, patch)
2010-07-30 11:16 UTC, Eduardo Lima Mitev
committed Details | Review

Description Eduardo Lima Mitev 2010-07-27 10:04:57 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.
Comment 1 Eduardo Lima Mitev 2010-07-27 15:46:50 UTC
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
Comment 2 Tomeu Vizoso 2010-07-29 09:34:51 UTC
+ * @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!
Comment 3 Allison Karlitskaya (desrt) 2010-07-29 10:27:04 UTC
also, please never attach patches as .tar.gz.  it prevents us using the review system.
Comment 4 Eduardo Lima Mitev 2010-07-29 13:09:47 UTC
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.
Comment 5 Allison Karlitskaya (desrt) 2010-07-29 14:51:21 UTC
Thanks for the patch.

Closing this bug now, since you pushed it.
Comment 6 Eduardo Lima Mitev 2010-07-29 15:00:51 UTC
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.
Comment 7 Eduardo Lima Mitev 2010-07-30 11:16:20 UTC
Created attachment 166823 [details] [review]
patch adding more annotations
Comment 8 Matthias Clasen 2010-08-05 05:27:46 UTC
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 ?
Comment 9 Eduardo Lima Mitev 2010-08-09 11:32:33 UTC
(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.
Comment 10 Eduardo Lima Mitev 2010-08-24 08:46:59 UTC
Anybody against committing this bug? perhaps I should wait people to return from holidays for more review.
Comment 11 Colin Walters 2010-09-15 21:04:12 UTC
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.
Comment 12 Eduardo Lima Mitev 2010-09-17 08:39:56 UTC
(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
Comment 13 Bug flys 2011-06-07 08:42:46 UTC
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):
  • File "<pyshell#55>", line 1 in <module>
    stream.read(10, None)
  • File "/usr/lib/python2.7/dist-packages/gi/types.py", line 44 in function
    return info.invoke(*args)
TypeError: read() takes exactly 4 argument(s) (3 given)

Comment 14 Dan Winship 2011-08-23 23:57:20 UTC
Most of the annotations from this patch had since been added by other
people, but I committed the remaining ones.