Created attachment 215222 [details] [review]
GVariant: Make g_variant_new_from_bytes() public, add more GBytes API

(In reply to comment #0)
> While we have the patient open, add g_variant_new_byte_array() which
> accepts GBytes.  The main advantage of this is that we don't have to
> do a copy of the data.

g_variant_new_from_bytes uses the GBytes as its storage -- there is already no copy made.

(In reply to comment #2)
> (In reply to comment #0)
> > While we have the patient open, add g_variant_new_byte_array() which
> > accepts GBytes.  The main advantage of this is that we don't have to
> > do a copy of the data.
> 
> g_variant_new_from_bytes uses the GBytes as its storage -- there is already no
> copy made.

Yes, but g_variant_new_byte_array() is different; it is really a shorthand for:

g_variant_new_from_data (G_VARIANT_TYPE ("ay"), buf, bytes_read, FALSE, g_free, buf);

under the assumption that your "buf" was allocated with g_malloc().

I've seen some code that uses g_variant_new_fixed_array() for byte arrays, but this is less efficient as it incurs a copy, which is unnecessary if you already had the data as a GBytes.

I know this is confusing because there's two GBytes here:

1) "variant data": When we want to construct a GVariant of any format from a GBytes, or turn an existing variant into GBytes 
2) "variants of type ay": A special case of 1), but one I happen to often use in my code, and I've seen other projects using g_variant_new_fixed_array() for this, which as mentioned above is less efficient if one already has a GBytes.

Created attachment 215225 [details] [review]
GVariant: Make g_variant_new_from_bytes() public, add more GBytes API

Just return a ref to our internal GBytes in
g_variant_get_data_as_bytes(), rather than making a new GBytes.

Review of attachment 215225 [details] [review]:

I don't like the new_byte_array() function.  It's just as easy to do this with a call to new_from_bytes and a type...

(In reply to comment #6)
> Review of attachment 215225 [details] [review]:
> 
> I don't like the new_byte_array() function.  It's just as easy to do this with
> a call to new_from_bytes and a type...

...and TRUE for trusted, as the implementation does.  However, many GVariant users may not be aware of the details of implementation, and don't know that an "ay" GVariant is literally just the data.  At least Will Thompson wasn't, and he's a very experienced hacker.

Having an API call to do this where one would expect to find it (next to the other fixed array functions) helps a lot.

I guess other factors in the cost/benefit analysis are:

1) How many programs make byte arrays (as distinct from bytestrings)
2) For those, how large are they?

Dunno.  I have two components (ostree, and one not public) which could use this.  See 

http://git.gnome.org/browse/ostree/tree/src/libotutil/ot-variant-utils.c?id=90aff4f2f6d604a5c3680a22df2e07c60d4a4d9d#n32
which is used in e.g.
http://git.gnome.org/browse/ostree/tree/src/libostree/ostree-core.c?id=90aff4f2f6d604a5c3680a22df2e07c60d4a4d9d#n866

I could split this in two parts if that helps.

Created attachment 227064 [details] [review]
gvariant: Make g_variant_new_from_bytes() public

Now that GBytes has been made public, we should make
g_variant_new_from_bytes() public too.

Add g_variant_get_data_as_bytes() to match.

Created attachment 227065 [details] [review]
gvariant: add g_variant_new_byte_array()

This is a convenience call around g_variant_new_from_bytes() for the
common case of wanting to convert a GBytes to a GVariant of type 'ay'.

Comment on attachment 227064 [details] [review]
gvariant: Make g_variant_new_from_bytes() public

Attachment 227064 [details] pushed as 4fb2d73 - gvariant: Make g_variant_new_from_bytes() public

I've split the patches, updated the since tags and pushed the first one (since
this half was quite obviously useful).

I'm still not convinced on the second one...

With having both _new_fixed_array() and _new_from_bytes(), I think that _new_byte_array() would be quite redundant (and also, I would not expect this to be a GBytes based function).  Let's call this bug finished with the one addition.