GNOME Bugzilla – Bug 765063
Update annotations for gio
Last modified: 2017-09-21 11:16:33 UTC
Created attachment 326027 [details] [review] Add annotations relating to lengths of arrays, change char* to gchar* for a string Add annotations to gio to enhance the API. In particular, there were some missing length annotations (either array length= or fixed-size=). Additionally, one update to change an argument's type from char* to gchar*, as it is a string and therefore zero-terminated. I will also attach a patch file for gobject-introspection's gir/gio.c file, containing the corresponding updates to the documentation.
Created attachment 326028 [details] [review] Update to gobject-introspection gio-2.0.c file Corresponding changes to gobject-introspection.
Review of attachment 326028 [details] [review]: No need for this; it’s easier to just regenerate the file when pushing.
Review of attachment 326027 [details] [review]: ::: gio/gdbusutils.c @@ +326,3 @@ */ gboolean +g_dbus_is_guid (const char *string) This change is unnecessary. char and gchar are equivalent. ::: gio/gfile.h @@ +602,3 @@ const gchar *cwd); GLIB_AVAILABLE_IN_2_32 +GFile * g_file_new_tmp (const gchar *tmpl, This change is unnecessary. char and gchar are equivalent.
Review of attachment 326027 [details] [review]: ::: gio/gtlspassword.c @@ +265,3 @@ * g_tls_password_set_value: * @password: a #GTlsPassword object + * @value: (array length=length) the new password value The same change needs to be made for g_tls_password_set_value_full().
Emmanuele, what are your thoughts on the potential introspection ABI breaks here? I’m tempted to update and push this patch, and revert it if anything breaks in Continuous (but otherwise live with the ABI change if nothing breaks).
gchar may be a typedef'd alias for char, but my impression was that gchar has extra implied properties that char may not have. According to the "Default Annotations:" paragraph under <https://wiki.gnome.org/Projects/GObjectIntrospection/Annotations#GObject-Introspection_annotations>, "gchar* means (type utf8) (transfer full)". Then under <https://wiki.gnome.org/Projects/GObjectIntrospection/Annotations#Default_Basic_Types>, "utf8" is further described as "string encoded in utf8". I assume that "string" here means that the valid part is terminated by a Unicode NULL character (U+0000). The "Default Annotations:" paragraph mentions nothing about default annotations for "char*"; it only mentions "gchar*". Should I assume that "char*" is also implicitly "(type utf8) (transfer full)"? Will GObject introspection tools that generate bindings always treat "char*" in this way? Or are these default annotations applied only to "gchar*" when spelled in that manner, not when spelled as the otherwise-equivalent "char*"?
(In reply to Ben Liblit from comment #6) > gchar may be a typedef'd alias for char, but my impression was that gchar > has extra implied properties that char may not have. As far as I can remember, they’re treated equivalently (including for the default annotations). > I assume that "string" here means that the valid part is > terminated by a Unicode NULL character (U+0000). Yup. > The "Default Annotations:" paragraph mentions nothing about default > annotations for "char*"; it only mentions "gchar*". Should I assume that > "char*" is also implicitly "(type utf8) (transfer full)"? Will GObject > introspection tools that generate bindings always treat "char*" in this way? > Or are these default annotations applied only to "gchar*" when spelled in > that manner, not when spelled as the otherwise-equivalent "char*"? IIRC they are equivalent, but the only way to be sure is to test it — see if the generated .gir file changes if you change from gchar* to char* or vice-versa.
Sorry for the delay on this. I went ahead and tested, and found that gchar* and char* do, indeed, generate the same .gir file (except for the name of the type - char* or gchar*). However, while the documentation indicates that gchar*'s default annotatation is "(type utf8) (transfer full)" as Ben said, the actual annotation that gets created is "(type utf8) (transfer none)". I will be submitting a separate bug report to document this. I'm not sure which annotation *should* be used here, but the documentation should reflect what the default annotations actually are.
(In reply to Alisa Maas from comment #8) > Sorry for the delay on this. I went ahead and tested, and found that gchar* > and char* do, indeed, generate the same .gir file (except for the name of > the type - char* or gchar*). However, while the documentation indicates that > gchar*'s default annotatation is "(type utf8) (transfer full)" as Ben said, > the actual annotation that gets created is "(type utf8) (transfer none)". I > will be submitting a separate bug report to document this. I'm not sure > which annotation *should* be used here, but the documentation should reflect > what the default annotations actually are. iirc the defaults are (type utf8) (transfer none) for `const gchar *`, and (type utf8) (transfer full) for `gchar *`. Which documentation contradicts this, or doesn’t make it clear? If you could update your patch to address the review issues I’ll get it pushed.
Review of attachment 326027 [details] [review]: ::: gio/gdatainputstream.c @@ +1292,3 @@ * g_data_input_stream_read_upto: * @stream: a #GDataInputStream + * @stop_chars: (array length=stop_chars_len) characters to terminate the read Also, you need a second colon after the `)` (in all of the chunks of the patch). i.e. ``` * @stop_chars: (array length=stop_chars_len): characters to terminate the read ```
I'll correct the patches and attach a new file soon. You're correct that the default for `const gchar *` is stated to be `(type utf8) (transfer none)` and the default for `gchar *` is stated to be `(type utf8) (transfer full)`. However, my test showed that using a `gchar *` provided the annotation `(type utf8) (transfer none)`. I opened a bug which has the program I used to cause this behavior (a modified version of the tutorial code) here: https://bugzilla.gnome.org/show_bug.cgi?id=787812. This bug report also has a link to the documentation that states the defaults as you said them.
Great, thanks. Hopefully one of the gobject-introspection maintainers will look at bug #787812 soon.
Created attachment 360158 [details] [review] Revised patch
Review of attachment 360158 [details] [review]: Thanks ::: gio/gtlspassword.c @@ +265,3 @@ * g_tls_password_set_value: * @password: a #GTlsPassword object + * @value: (array length=length) the new password value This is still missing a colon, but I’ll fix that before pushing.
I also added a commit message before pushing. Thanks for the patch.
Turns out (array length=…) on a gchar* parameter causes the GIR file to be generated containing an array of strings, rather than an array of characters (or a string with a noted length). Reopening while we work out what to do here. I’ve partially reverted the patch as commit 09796b3ccc8c7b9785fe66887e20f506cb58c7a4.
It doesn’t look like there’s an easy way of adding back the reverted annotations without breaking other annotated APIs and breaking the GIR API. Resolving as FIXED again, since at least some of the annotation changes got in.