Created attachment 203159 [details] [review]
GBytes: add a size argument to g_bytes_get_data

 * An out size argument so that this is more easily bindable
   by gobject-introspection.

Review of attachment 203159 [details] [review]:

minor stylistic issues; other than that, +1 from me.

::: glib/gbytes.c
@@ +213,3 @@
  * g_bytes_get_data:
  * @bytes: a #GBytes
+ * @size: (allow-none): location to return size of byte data

this should be (out) at the very least; I think you may need to force the caller-allocates annotation as well. (allow-none) is implied for (out) arguments.

@@ +219,3 @@
  * This function will always return the same pointer for a given #GBytes.
  *
+ * Returns: (array length=size) (type guchar): a pointer to the byte data

instead of (type guchar), I'd use (type guint8) here; I think all bindings have standardized on that.

::: glib/tests/array-test.c
@@ +790,3 @@
   g_assert (bytes != NULL);
   g_assert_cmpuint (g_bytes_get_size (bytes), ==, 11);
+  g_assert (g_bytes_get_data (bytes, NULL) == memory);

would be nice to test the out size here as well, to avoid going out of sync, but it's just my anal-retentiveness for test units speaking.

::: glib/tests/bytes.c
@@ +30,3 @@
   bytes = g_bytes_new (data, 4);
   g_assert (bytes != NULL);
+  g_assert (g_bytes_get_data (bytes, NULL) != data);

same as the bit in array-test.c: testing the out parameter in at least a couple of places would be nice for completeness.

(In reply to comment #2)
> Review of attachment 203159 [details] [review]:
> 
> minor stylistic issues; other than that, +1 from me.

Thanks for looking it over.

> ::: glib/gbytes.c
> @@ +213,3 @@
>   * g_bytes_get_data:
>   * @bytes: a #GBytes
> + * @size: (allow-none): location to return size of byte data
> 
> this should be (out) at the very least; I think you may need to force the
> caller-allocates annotation as well. (allow-none) is implied for (out)
> arguments.

Added (out), although I had imagined that (array length=PARAM) implied (out) :) I don't think (allow-none) should be implied, as not all APIs allow NULL for an out parameter. 

> @@ +219,3 @@
>   * This function will always return the same pointer for a given #GBytes.
>   *
> + * Returns: (array length=size) (type guchar): a pointer to the byte data
> 
> instead of (type guchar), I'd use (type guint8) here; I think all bindings have
> standardized on that.

Done.

> ::: glib/tests/array-test.c
> @@ +790,3 @@
>    g_assert (bytes != NULL);
>    g_assert_cmpuint (g_bytes_get_size (bytes), ==, 11);
> +  g_assert (g_bytes_get_data (bytes, NULL) == memory);
> 
> would be nice to test the out size here as well, to avoid going out of sync,
> but it's just my anal-retentiveness for test units speaking.

Good done.

> ::: glib/tests/bytes.c
> @@ +30,3 @@
>    bytes = g_bytes_new (data, 4);
>    g_assert (bytes != NULL);
> +  g_assert (g_bytes_get_data (bytes, NULL) != data);
> 
> same as the bit in array-test.c: testing the out parameter in at least a couple
> of places would be nice for completeness.

Added.

And merged into master.

(In reply to comment #3)
> (In reply to comment #2)

> Added (out), although I had imagined that (array length=PARAM) implied (out) :)
> I don't think (allow-none) should be implied, as not all APIs allow NULL for an
> out parameter. 

what I meant was that all bindings will pass an out argument to the C function, and then decide whether or not to pass it back to the caller based on the calling conventions of the language itself; this is especially true for integral types - allow-none is more geared towards pointer types, like object instances, or boxed types passed as in arguments.

anyway, minor nitpicks - and I'm pretty sure language bindings authors will fix the annotations to their liking if need be.

Yeah makes sense.