Re: Reviewing GVariant, part II

[Ryan Lortie <desrt desrt ca>]

Hi Matthias

Thanks for the further comments.

I'm on holiday until the end of the year at this point.  I'll get the
ball rolling again in early January.

Cheers

On Sun, 2009-12-06 at 17:01 -0500, Matthias Clasen wrote:
> 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
>