Reviewing GVariant, part II

[Matthias Clasen <matthias clasen gmail com>]

Today I had another go at reviewing the gvariant branch, this time
starting from the tests.

First, I didn't see any documentation improvements after my last
review attempt...are those coming, or are we stuck here ?

Here's my observations from looking at the tests:

- Some gvariant tests are GPL3, which is ok, I think. But the top
comments refer to COPYING, which is LGPLv2. Other tests have no top
comment at all, which you may want to fix...

- it would be great to add a comment to each test, explaining what it
is supposed to test. It is reasonably clear for some, but e.g. big,
fuzz and random could do with an explanation.

- gvariant-endian.c test calls g_variant_load like this:

  value = g_variant_load (NULL, data, sizeof data,
                          G_BIG_ENDIAN | G_VARIANT_LAZY_BYTESWAP);

Comparing this to the docs of g_variant_load, it is not documented
that you can pass NULL for the type. Furthermore, the flags are not
documented at all, and I consider using G_BIG_ENDIAN between the
G_VARIANT_ flags at least bad taste. Just add a G_VARIANT_BIG_ENDIAN
flag, it can be the same bit as G_BIG_ENDIAN if you are into that kind
of thing...

- The OPAQUE_TYPE__ prefixes should go away, to match existing GLib style.

- The docs need to explain the concept of floating - you would not
have to do that if you made GVariant a GObject...

- The docs should give some guidance as to when calling flatten is
advised. e.g. I don't see why gvariant-markup.c:check_verbatim is
calling g_variant_flatten.

- The docs for the format string in g_variant_get are really totally
insufficient. I have no idea what "^a&s" means, and the docs in no way
help me understand it.

- gvariant-core.c misses a big comment explaining the condition machinery.


I'm going to hold off doing further review until the docs get into better shape.


Matthias