Bug 30428

It turns out dbus_g_value_build_g_variant didn't work for all types and values - it fell over on empty arrays, and on any GArray (because GArray didn't implement all the specialized collection methods) - so I fixed that in the same branch, and added regression tests.

Review feedback:

+      if (dg_size == gv_size && 0)

Is this what you mean?

+                gchar signature[] = { type_char, '\0' };
+
+                for (i = 0; i < n; i++)
+                  {
+                    gchar *s;
+
+                    g_variant_get_child (variant, i, signature, &s);
+                    g_ptr_array_add (pa, s);

Why not use g_variant_get_string() ?

+      dbus_g_value_parse_variant_by_type (
+          variant == NULL ? NULL : g_variant_get_child_value (variant, i),
+          inner_type, va->values + i);

I find 'va->values + i' to be obscure. Wouldn't &va->values[i] be clearer?

+      case G_VARIANT_CLASS_HANDLE:
+      case G_VARIANT_CLASS_MAYBE:
+        g_critical ("unhandled GVariantClass %d",
+            g_variant_classify (variant));

Wouldn't '%c' be more useful?

+ * It is an error if @variant contains any GDBus extensions not supported by
+ * dbus-glib, including handles (file descriptor passing) and 'maybe' types.

s/GDBus/GVariant/ ?

(In reply to comment #2)
> Review feedback:
> 
> +      if (dg_size == gv_size && 0)
> 
> Is this what you mean?

Oops, no, that was to get more thorough testing for the "not just memcpy" case. I'll delete the " && 0" to reinstate the fast-path :-)

> +                gchar signature[] = { type_char, '\0' };
> +
> +                for (i = 0; i < n; i++)
> +                  {
> +                    gchar *s;
> +
> +                    g_variant_get_child (variant, i, signature, &s);
> +                    g_ptr_array_add (pa, s);
> 
> Why not use g_variant_get_string() ?

It'd have to be:

    g_variant_dup_string (g_variant_get_child_value (variant, i), NULL)

which I think is more unwieldy. g_variant_get_child effectively combines g_variant_get_child_value with g_variant_get.

> +      dbus_g_value_parse_variant_by_type (
> +          variant == NULL ? NULL : g_variant_get_child_value (variant, i),
> +          inner_type, va->values + i);
> 
> I find 'va->values + i' to be obscure. Wouldn't &va->values[i] be clearer?

Possibly. The &x[i] form annoys me because it has more punctuation and more operations than it needs (to a reader - a decent compiler should produce the same code either way), but the x + i form does rely on C's array/pointer duality.

> +      case G_VARIANT_CLASS_HANDLE:
> +      case G_VARIANT_CLASS_MAYBE:
> +        g_critical ("unhandled GVariantClass %d",
> +            g_variant_classify (variant));
> 
> Wouldn't '%c' be more useful?

I'd prefer to avoid logging non-UTF-8 on (sufficiently spectacular) failures; this is only reachable in programmer-error cases anyway.

> + * It is an error if @variant contains any GDBus extensions not supported by
> + * dbus-glib, including handles (file descriptor passing) and 'maybe' types.
> 
> s/GDBus/GVariant/ ?

True, I'll fix that.

(In reply to comment #3)

> > +                gchar signature[] = { type_char, '\0' };
> > +
> > +                for (i = 0; i < n; i++)
> > +                  {
> > +                    gchar *s;
> > +
> > +                    g_variant_get_child (variant, i, signature, &s);
> > +                    g_ptr_array_add (pa, s);
> > 
> > Why not use g_variant_get_string() ?
> 
> It'd have to be:
> 
>     g_variant_dup_string (g_variant_get_child_value (variant, i), NULL)
> 
> which I think is more unwieldy. g_variant_get_child effectively combines
> g_variant_get_child_value with g_variant_get.

Personally, I disagree. I had to think significantly more than seemed appropriate to work out what you were trying to do here.

> > +      dbus_g_value_parse_variant_by_type (
> > +          variant == NULL ? NULL : g_variant_get_child_value (variant, i),
> > +          inner_type, va->values + i);
> > 
> > I find 'va->values + i' to be obscure. Wouldn't &va->values[i] be clearer?
> 
> Possibly. The &x[i] form annoys me because it has more punctuation and more
> operations than it needs (to a reader - a decent compiler should produce the
> same code either way), but the x + i form does rely on C's array/pointer
> duality.

& makes it explicit that you're passing a pointer. I agree the compiler should produce the same code, and that what matters is the clarity to the reader (I read `&values[i]` as passing the pointer to the i-th member of the array; whereas I'm not sure if `values + i` is a pointer or an integer without first discovering the type of @values.

> > +      case G_VARIANT_CLASS_HANDLE:
> > +      case G_VARIANT_CLASS_MAYBE:
> > +        g_critical ("unhandled GVariantClass %d",
> > +            g_variant_classify (variant));
> > 
> > Wouldn't '%c' be more useful?
> 
> I'd prefer to avoid logging non-UTF-8 on (sufficiently spectacular) failures;
> this is only reachable in programmer-error cases anyway.

CLAMP() could be of use here? Include both %c and %d? Even if it's programmer error, it seems mean to make someone look at `man 7 ascii`.

(In reply to comment #4)

> > > +                gchar signature[] = { type_char, '\0' };
> > > +
> > > +                for (i = 0; i < n; i++)
> > > +                  {
> > > +                    gchar *s;
> > > +
> > > +                    g_variant_get_child (variant, i, signature, &s);
> > > +                    g_ptr_array_add (pa, s);
> > > 
> > > Why not use g_variant_get_string() ?
> > 
> > It'd have to be:
> > 
> >     g_variant_dup_string (g_variant_get_child_value (variant, i), NULL)
> > 
> > which I think is more unwieldy. g_variant_get_child effectively combines
> > g_variant_get_child_value with g_variant_get.
> 
> Personally, I disagree. I had to think significantly more than seemed
> appropriate to work out what you were trying to do here.

Alternatively I think this could also be:

while (g_variant_iter_loop (&iter, "?", &variant))
  g_ptr_array_add (g_variant_dup_string (variant, NULL))

Though I wonder if that's any easier to parse...

(In reply to comment #3)
> I'll delete the " && 0" to reinstate the fast-path :-)

Fixed in the branch

> > Why not use g_variant_get_string() ?
> g_variant_get_child effectively combines
> g_variant_get_child_value with g_variant_get.

Not changed, let me know if you're unconvinced by my reasoning

> Possibly. The &x[i] form annoys me

Changed in the branch anyway, perhaps explicit is better than implicit

> > Wouldn't '%c' be more useful?
> 
> I'd prefer to avoid logging non-UTF-8

Not changed, let me know if you're unconvinced by my reasoning

> > s/GDBus/GVariant/ ?

Fixed in the branch

(Gah, mid-air collisions...)

(In reply to comment #4)
> > It'd have to be:
> > 
> >     g_variant_dup_string (g_variant_get_child_value (variant, i), NULL)
> > 
> > which I think is more unwieldy.

Having tried implementing this, it doesn't look as bad as I'd feared, so: fixed in the branch.

> g_variant_get_child effectively combines
> g_variant_get_child_value with g_variant_get.

This naming is unfortunate: g_variant_get_child gets the value of the child that g_variant_get_child_value would return, so in a sense, the naming is backwards :-)

> CLAMP() could be of use here? Include both %c and %d?

Good thinking, fixed.

Looks good.

Thanks, fixed in git for 0.90.