Bug 28715

Implementing branch: http://git.collabora.co.uk/?p=user/danni/dbus-glib.git;a=shortlog;h=refs/heads/gvalue-to-gvariant

Tests coming soon.

Branch is now updated with tests.

(In reply to comment #2)
> Branch is now updated with tests.

As I said on IRC, I'm suspicious about the non-order-independence of equality for hash tables... Since you've filed a bug maybe you could mention the bug in the test?

+ * dbus_g_type_get_struct(). Providing any other type is a programming error
+ * (including as a child type).

The parens aren't clear to me. I think

  “Passing a value containing any other type — including as a child, such as a value in an a{sv} — is a programming error.”

might be clearer? And in any case:

+      g_error ("%s: Unknown type: %s", G_STRFUNC, g_type_name (type));

should probably g_return_val_if_reached (NULL); rather than aborting your program?

(In reply to comment #3)
> (In reply to comment #2)
> > Branch is now updated with tests.
> 
> As I said on IRC, I'm suspicious about the non-order-independence of equality
> for hash tables... Since you've filed a bug maybe you could mention the bug in
> the test?

Yeah. I can add that in. For reference: https://bugzilla.gnome.org/show_bug.cgi?id=622590

> + * dbus_g_type_get_struct(). Providing any other type is a programming error
> + * (including as a child type).
> 
> The parens aren't clear to me. I think
> 
>   “Passing a value containing any other type — including as a child, such as a
> value in an a{sv} — is a programming error.”
> 
> might be clearer? And in any case:
> 
> +      g_error ("%s: Unknown type: %s", G_STRFUNC, g_type_name (type));
> 
> should probably g_return_val_if_reached (NULL); rather than aborting your
> program?

I don't like the g_error() but this seems tricky, because it's not so easy to break out of the recursive function in a clean way. Suggestions are appreciated.

(In reply to comment #4)
> > 
> > As I said on IRC, I'm suspicious about the non-order-independence of equality
> > for hash tables... Since you've filed a bug maybe you could mention the bug in
> > the test?
> 
> Yeah. I can add that in. For reference:
> https://bugzilla.gnome.org/show_bug.cgi?id=622590

It seems that they're not interested in changing g_variant_equal, so I've written my own GVariant comparison function and included it in the test.

(In reply to comment #5)
> (In reply to comment #4)
> > > 
> > > As I said on IRC, I'm suspicious about the non-order-independence of equality
> > > for hash tables... Since you've filed a bug maybe you could mention the bug in
> > > the test?
> > 
> > Yeah. I can add that in. For reference:
> > https://bugzilla.gnome.org/show_bug.cgi?id=622590
> 
> It seems that they're not interested in changing g_variant_equal, so I've
> written my own GVariant comparison function and included it in the test.

I think this looks fine. I think it might be possible to write two different dictionaries that it considers equivalent (by giving the same key more than one value) but I can't find one off-hand, and it's just test code, so.

> +dbus_g_value_build_g_variant (const GValue *value)

Not a big deal, but I'd have done this as a switch() over constant types (G_TYPE_BOOLEAN etc.), with the un-switch()able types (G_TYPE_STRV, DBUS_TYPE_* and non-GValue containers) in the default case. This gets the simple cases out of the way early (and with more efficient machine code, if you care about such things), then goes into the more complex cases.

You've missed out DBUS_TYPE_G_SIGNATURE, I think?

> +PKG_CHECK_MODULES(DBUS_GLIB, gobject-2.0 >= 2.24, have_glib=yes, have_glib=no)

For future reference, I'd have preferred this as the first patch in the series, so that after each patch is applied, the project compiles and works.

Don't we need to add gio here now?

Please check that dbus-glib still compiles if you use "./configure LDFLAGS=-Wl,--no-add-needed", which matches the (patched) default behaviour of ld in Fedora, and the default behaviour of gold(1) everywhere. You might also need to put $(DBUS_GLIB_LIBS) in the LDADD variables in core/Makefile.am to make that work.

At some point we'll probably want a dbus_g_variant_build_value() that does the converse.

(In reply to comment #7)

> You've missed out DBUS_TYPE_G_SIGNATURE, I think?

Fixed.

> > +PKG_CHECK_MODULES(DBUS_GLIB, gobject-2.0 >= 2.24, have_glib=yes, have_glib=no)
> 
> For future reference, I'd have preferred this as the first patch in the series,
> so that after each patch is applied, the project compiles and works.

Fixed.

> Don't we need to add gio here now?

GVariant is in GLib not GIO. So I don't think so.

> Please check that dbus-glib still compiles if you use "./configure
> LDFLAGS=-Wl,--no-add-needed", which matches the (patched) default behaviour of
> ld in Fedora, and the default behaviour of gold(1) everywhere. You might also
> need to put $(DBUS_GLIB_LIBS) in the LDADD variables in core/Makefile.am to
> make that work.

I couldn't get this to work, but it seemed to be breaking on things that weren't related to my patch, so I must be doing something wrong.

(In reply to comment #8)
> > Don't we need to add gio here now?
> 
> GVariant is in GLib not GIO. So I don't think so.

If that's the case, removing "#include <gio/gio.h>" throughout, and verifying that it still all works, is an alternative way to shut me up :-)

> > Please check that dbus-glib still compiles if you use "./configure
> > LDFLAGS=-Wl,--no-add-needed", which matches the (patched) default behaviour of
> > ld in Fedora, and the default behaviour of gold(1) everywhere. You might also
> > need to put $(DBUS_GLIB_LIBS) in the LDADD variables in core/Makefile.am to
> > make that work.
> 
> I couldn't get this to work, but it seemed to be breaking on things that
> weren't related to my patch, so I must be doing something wrong.

It's possible that dbus-glib was broken for -Wl,--no-add-needed already; Fedora probably doesn't build the tests (whereas in telepathy-glib we build the tests in a normal build, even if not running 'make check') so they haven't noticed yet. So, if it's not a regression, don't worry about it.

(In reply to comment #9)
> (In reply to comment #8)
> > > Don't we need to add gio here now?
> > 
> > GVariant is in GLib not GIO. So I don't think so.
> 
> If that's the case, removing "#include <gio/gio.h>" throughout, and verifying
> that it still all works, is an alternative way to shut me up :-)

Not sure how that got in there. Fixed.

Looks good to me!

http://cgit.freedesktop.org/dbus/dbus-glib/commit/?id=9440209e203cccef158904800ea623568637a71e