GNOME Bugzilla – Bug 732561
Documentation and typing improvements for GVariant bytes
Last modified: 2018-05-24 16:42:58 UTC
Patches coming, as discussed with desrt on IRC on 2014-06-30. (In summary, the types of bytes, bytestrings and byte arrays in GVariant are mildly confusing, as they use gchar, guchar and guint8 in various places. We should replace guchar with guint8 where appropriate.)
Created attachment 279698 [details] [review] gvariant: Change type of ‘y’ variants from guchar to guint8 This fits better with the convention in the rest of GLib where arbitrary 8-bit values are represented as guint8, avoiding the potential confusing of a name which references ‘char’s. This is not an API break, as both guint8 and guchar are unconditionally typedeffed to unsigned char.
Created attachment 279699 [details] [review] gvariant: Document differences between GVariant bytestrings and arrays
Review of attachment 279698 [details] [review]: This is fine, as long as it doesn't cause the bindings to change...
Review of attachment 279699 [details] [review]: ::: docs/reference/glib/gvariant-text.xml @@ +566,3 @@ + constructs arrays of non-nul bytes (type '<literal>ay</literal>') with a nul terminator at the end. These are + normal C strings with no particular encoding enforced, so the bytes may not be valid UTF-8. Bytestrings are + semantically distinct from byte arrays (also type '<literal>ay</literal>') which are not nul-terminated and The notes about the common use of this to convey not-utf8-string data is useful, but I'm not sure about the rest. It almost falls into the "does it really need to be stated?" category. Saying that an array of bytes can contain 0 at arbitrary locations is almost too obvious to state. Saying it here, of all places, is just strange. Also the "not nul-terminated" part is false, because it very well could be. If anything, I'd prefer something more like "bytestrings are a special case of byte arrays; byte arrays, in the general case, can contain nul at any position, and need not end with nul". Even that is confusing, however, since the (even slightly) careless reader may assume that we're still talking about what we're talking about here (ie: bytestrings) and wonder why we're saying that they can contain nul, after all.... I'd also prefer if "^ay" was not mentioned during discussion of the text format -- maybe it would be better to improve the format string documentation instead.
(In reply to comment #3) > Review of attachment 279698 [details] [review]: > > This is fine, as long as it doesn't cause the bindings to change... The diff of GLib-2.0.gir is: $ diff -u GLib-2.0.gir GLib-2.0.new.gir --- GLib-2.0.gir 2014-07-04 14:18:57.470417000 +0100 +++ GLib-2.0.new.gir 2014-07-04 14:16:24.008825341 +0100 @@ -28872,7 +28872,7 @@ <parameters> <parameter name="value" transfer-ownership="none"> <doc xml:space="preserve">a #guint8 value</doc> - <type name="guint8" c:type="guchar"/> + <type name="guint8" c:type="guint8"/> </parameter> </parameters> </constructor> @@ -29939,8 +29939,8 @@ It is an error to call this function with a @value of any type other than %G_VARIANT_TYPE_BYTE.</doc> <return-value transfer-ownership="none"> - <doc xml:space="preserve">a #guchar</doc> - <type name="guint8" c:type="guchar"/> + <doc xml:space="preserve">a #guint8</doc> + <type name="guint8" c:type="guint8"/> </return-value> <parameters> <instance-parameter name="value" transfer-ownership="none"> @@ -30173,7 +30173,7 @@ the appropriate type: - %G_VARIANT_TYPE_INT16 (etc.): #gint16 (etc.) - %G_VARIANT_TYPE_BOOLEAN: #guchar (not #gboolean!) -- %G_VARIANT_TYPE_BYTE: #guchar +- %G_VARIANT_TYPE_BYTE: #guint8 - %G_VARIANT_TYPE_HANDLE: #guint32 - %G_VARIANT_TYPE_DOUBLE: #gdouble So the c:types change (as could be expected), but the main types don’t. OK to commit?
I'd prefer an ACK from someone who knows more about this sort of thing. Colin, maybe?
(In reply to comment #4) > Review of attachment 279699 [details] [review]: > > ::: docs/reference/glib/gvariant-text.xml > @@ +566,3 @@ > + constructs arrays of non-nul bytes (type '<literal>ay</literal>') with a > nul terminator at the end. These are > + normal C strings with no particular encoding enforced, so the bytes may > not be valid UTF-8. Bytestrings are > + semantically distinct from byte arrays (also type '<literal>ay</literal>') > which are not nul-terminated and > > The notes about the common use of this to convey not-utf8-string data is > useful, but I'm not sure about the rest. It almost falls into the "does it > really need to be stated?" category. I was trying to clarify the differences between the two, rather than stating their properties separately. I guess that can come across as stating the obvious, but I think it’s better to be explicit. > If anything, I'd prefer something more like … I’m not sure I understand where you want that text spliced in. Perhaps it would be easier (and faster) if you rewrote the patch yourself.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/896.