A few comments on GVariant

[Christian Persch <chpe gnome org>]

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 :-)

- 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...

- 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?

- 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 :-)

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

- 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.

  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?

- 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?

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

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

- 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.

- 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 ?

- 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

- 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? 

  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().

That's it for now.

Regards,
	Christian