Re: A few comments on GVariant

[Ryan Lortie <desrt desrt ca>]

Hi Christian

On Mon, 2009-11-30 at 13:27 +0100, Christian Persch wrote:
> Hi;
> 
> I've been playing with the gvariant branch of glib a bit, and have a
> few questions / remarks / code comments. I hope they're marginally
> useful :-)

Thanks so much for this feedback.  It's extremely valuable.

> - There really should be a GVariant tutorial, not just the API docs. If
>   I hadn't at least had a vague idea what GVariant does, I would have
>   been completely helpless with the API docs. Even so, I had to proceed
>   by trial-and-error by copying what the tests do...

I totally agree with this.  I've even considered using example/tutorial
style documentation in place of full proper reference documentation in
some places since it's difficult to formally describe, for example,
GVariant format strings, but quite easy to show how to use them by
example.

> - Type strings: to construct them, you need to remember which character
>   is which type. There are the G_VARIANT_TYPE_* defines, but they are
>   non-concatenable, i.e. you can't construct a type string like
>   G_VARIANT_TYPE_A G_VARIANT_TYPE_B but have to use "ab" which makes
>   typos uncaught at the compiler stage. Could there also be
>   concatenable #defines for these?

My attitude about this is that you should "just deal".  Basically,
reserve some extra brain cells for it and memorise it -- just like
printf.  The fact that it's the same as the DBus type system means that
you can even share those brain cells. :)

Consider if we had to write things like:

  g_strdup_print ("int: " G_PRINTF_TYPE_INTEGER ", "
                  "float: " G_PRINTF_TYPE_FLOAT "\n", 234, 2.5);

I think this really injures the readability.

> - g_variant_type_matches docs say: "This function defines a bounded
>   join-semilattice over GVariantType for which G_VARIANT_TYPE_ANY is
>   top." 
>   If you do want to go into that much detail, there should be a
>   more complete definition of how that ... thing ... is constructed :-)

It's a bit of an easter-egg in the docs.  I don't mind too much to
remove it.

> - g_variant_builder_add[_value]: should this return the
>   passed-in GVariantBuilder* instead of void, so one can chain calls?

That's an interesting proposal.  It's very evil in terms of refcounts,
but the refcounting semantics of GVariantBuilder are fairly unbindable
at this point anyway, so who cares about some extra magic? :)

Can you give me a good example of a use case you have here?

> - collection behaviour of g_variant_new: "a" types are collected as
>   GVariantBuilder. This seems very inconvenient to me, since one cannot
>   then build the whole thing in just the one call.

But there are modifiers.  Arrays of strings can be converted from a strv
with '^as'.  Arrays of constant-sized items (ie: ints, floats, and even
structure types of these things) can be constructed using, for example,
'&ai'.

Do you have another method you'd prefer for array construction?  Maybe
one that collects each array item from the varargs list?  Is that evil?

>   This appears to have been changed from earlier code; in the removed
>   glib/tests/gvariant-complex.c test, it was done this way:
> 
>   variant = g_variant_new ("(a{si}s)",
>                            3,
>                              "zero", 0,
>                              "one",  1,
>                              "two",  2,
>                            "");
> 
>   IMHO a vastly more convenient way to build an array in one step, in
>   case one already knows how many elements one has and what they are.
> 
>   Could there again be a convenience method like this?

mmm.  Yes.  It used to be like that, indeed.  The reason that I removed
it was because it was not possible to have a dual of it for
g_variant_get() and I'm trying to keep g_variant_new() as similar to
g_variant_get() as is possible.

Note that you can do in-place parsing of:

  "( { 'zero': 0, 'one': 1, 'two': 2 }, '' )"

if you prefer.

> - space efficiency: I was trying to build a variant containing some
>   data, and multiple dicts to get at the data from various keys. i.e
>   "a{sv}a{sv}a{sv}". I though that if I constructed the data
>   GVariants first, and then added these to the dicts (i.e.
>   v=g_variant_new(), add (key1, v) to dict1, add (key2, v) to dict2
>   etc), the resulting blob (g_variant_flatten
>   + get the data out) would contain that data only once instead of once
>   for each dict it was in. Well, it didn't work :-) 
>   Was that a naive/dumb thing to expect, or can this be made to work? 
> 
>   I ended up doing "ava{su}a{su}a{su}", first putting the data into
>   an array, then the index of the data in the array into the dicts.
>   Is that the best way for this?

This is the recommended way, yes.  If you have the data appearing
multiple times then it will appear multiple times.  There is no
compression here.

> - almost all functions are missing Since: markers; the Since: marked in
>   the G_TYPE_GVARIANT definition mistakenly says 2.14

Noted.

> - gvariant-*.c fail to #include "config.h"

What's the policy on this sort of thing?  Include everywhere or only
include if needed?

> - g_variant_markup_parser_end_element uses lots of
>   g_ascii_strto[u]ll/g_ascii_strtod but does no error- or overflow
>   checks? Other things are error-checked in there, so if this is
>   not-checked on purpose, a comment may be in order.

I'll look into that.

> - how about having a variant of g_variant_print that operates on a
>   provided (buffer, len) ?
> 
> - +#ifndef GSIZE_TO_LE
>   +#if GLIB_SIZEOF_SIZE_T == 4
>   +# define GSIZE_TO_LE GUINT32_TO_LE
>   +#else
>   +# define GSIZE_TO_LE GUINT64_TO_LE
>   +#endif
>   +#define GSIZE_FROM_LE GSIZE_TO_LE
>   +#endif
> 
>   Shouldn't these byteswapping macros be made public like the
>   existing repertoire in gtypes.h ?

Matthias?

> - there should be a convenience function that takes a dictionary
>   variant and turns it into a GHashTable (I was surprised that
>   g_variant_lookup[_value] just iterates over the whole thing until a
>   match is found)



> - the code uses literal 'X' / "X" in many places; IMHO using
>   G_VARIANT_TYPE_CLASS_X / G_VARIANT_TYPE_X would make the code more
>   readable / informative

All I can say to this is that it's easier to type this:

  case 'a': case 'b': case 'c': case 'd':

than:

  case G_VARIANT_TYPE_CLASS_ALPHA:
  case G_VARIANT_TYPE_CLASS_BETA:
  case G_VARIANT_TYPE_CLASS_GAMMA:
  case G_VARIANT_TYPE_CLASS_DELTA:

> - g_variant_parse_full is rather unusual in that it returns an error
>   not in a GError but a GVariantParseError. Could this be
>   implemented instead by fixing bug 124022
>   [https://bugzilla.gnome.org/show_bug.cgi?id=124022] and then using
>   that to put the extra data into a GError? 

Does this bug have any hope of being fixed any time soon?

I could also just eliminate the "fancy" error reporting, but I think it
can be really helpful (see below).

>   Actually, for what is this _full variant useful at all to have as a
>   public function — can't it just be private? It seems to only be used
>   in g_variant_parse().

The GSettings schema file parser calls the _full variant to parse
default values out of schema files.  The extra information is used to
tie-in to the error reporting system of that parser which does
Vala-style reporting of the lcation of errors.
                            ^^^^^^^

> That's it for now.

Thanks :)

Cheers