Merge lp://staging/~compiz-team/compiz/compiz.fix_1018602 into lp://staging/compiz/0.9.8

Oops. There is a problem under case TypeMatch:
array is no longer initialized with zeros because you changed calloc to malloc. So the free() loop after it could potentially free an invalid pointer if g_variant_iter_loop doesn't find exactly nItems.

Also, strdup(value) is not necessary if you use:
     while (g_variant_iter_next (iter, "s", &value))
  *arrayCounter++ = value;
and then change the free() calls below it to g_free().

On that note, the variable variantType should be removed from readListValue. Just use "s", "i", "b" and "d" instead. It will be smaller, faster and more readable.

Coolio, fixed the calloc issue, plugged a few other memleaks and fixed some gvariant errors I noticed.

I abstracted out the variantType stuff into a function - it should probably be transparent to the user as to the implementation detail of having to pass an arbitrary string to g_variant_get

Thanks for the tip on using g_variant_iter_next ... that's now fixed too.

The addition of compizconfigTypeToVariantType makes no sense.

We already have a tight coupling between:
    TYPE x;
and:
    g_variant_iter_loop (&iter, TYPESTRING, &x)

Using:
    g_variant_iter_loop (&iter, someVariable, &x)
only makes the code harder to read, with looser coupling and weaker cohesion.

Please remove compizconfigTypeToVariantType and just use:
    g_variant_iter_loop (&iter, "X", &x)

It's the same as:
    printf(secretFormatString, x, y, z);
It's too hard to read so adds maintenance risk.

Everyone would much prefer to read:
    printf("The results are: %d %s %f\n", x, y, z);

I think the printf analogy in this case doesn't apply here, the type string is merely an implementation detail - we know from the code what we want to extract already, and the type string is there to operate the valist API.

In any case, I'll change it.

Maybe. But back to the actual case of g_variant_iter_loop(I, F, V)... F and V are tightly coupled and need to match up so you should see the explicit value of F and type of V within the same block of code...

int v;
g_variant_iter_loop(&x, "i", &v)

I had another look at the code and variantType is more or less used as a placeholder so that copypasting was easier and so that we could return if there was no variant type for that setting. The fact that we are copypasting is a problem, but I modified the function so that it returns a boolean for whether or not we can read that type of setting.

It works OK, but I can see some potentially serious mistakes:

203: "f" should be "d"
(http://developer.gnome.org/glib/2.30/glib-GVariant.html#GVariantClass)

235: Missing g_free() loop:
    for (i = 0; i < nItems; i++)
        if (array[i])
            g_free(array[i]);

239: g_free() on calloc'd block (214). Either change g_free() to free(), or calloc() to g_malloc0().

> It works OK, but I can see some potentially serious mistakes:
>
> 203: "f" should be "d"
> (http://developer.gnome.org/glib/2.30/glib-GVariant.html#GVariantClass)

Thanks, fixed.

>
> 235: Missing g_free() loop:
> for (i = 0; i < nItems; i++)
> if (array[i])
> g_free(array[i]);

My understanding is that g_variant_iter_next returns a shallow copy of the array, so free () is sufficient in this case (unlike the previous code where we copied the values out of the array. Valgrind reports there are no leaks here.

>
> 239: g_free() on calloc'd block (214). Either change g_free() to free(), or
> calloc() to g_malloc0().

Fixed, although I think g_malloc is a wrapper around malloc itself (eg, doesn't use g_slice_alloc)

No, you still need the g_free loop to free each array element after g_variant_iter_next:
  http://developer.gnome.org/glib/2.32/glib-GVariant.html#g-variant-iter-next
It's possible valgrind doesn't see the leak because the memory is managed by glib and freed at exit. But it's still a leak even still.

Also, I was referring to g_malloc0() to replace calloc. Not g_malloc(). :)

You're right, I was confusing the array with the values themselves. Fixing it now.

Still needs fixing. Please don't click resubmit till after you've pushed the changes.

Sorry, my fault. I commented on an old version of the proposal. But still needs fixing.

Fixed that, plugged other memleaks.