Bug 28715 - dbus_g_value_build_g_variant
dbus_g_value_build_g_variant
Status: RESOLVED FIXED
Product: dbus
Classification: Unclassified
Component: GLib
unspecified
Other All
: medium normal
Assigned To: Danielle Madeley
John (J5) Palmieri
http://git.collabora.co.uk/?p=user/da...
r+
: patch
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-24 03:52 UTC by Danielle Madeley
Modified: 2010-08-05 02:16 UTC (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Danielle Madeley 2010-06-24 03:52:01 UTC
http://lists.freedesktop.org/archives/dbus/2010-June/013001.html

"""
One of the problems we have with applying GObject-Introspection to
telepathy-glib is that we have no way to introspect the a{sv}
GHashTables exposed in tp-glib's API if the variant value is more
complex than a simple type, since the GValue used to store the variant
does not contain the full type.

i.e. introspection works for { 'one': <1> }, but not { 'one': <(1,
'One')> }

dbus-glib however stores complete type information for registered
container types, which we can look up to build a GVariant of the complex
type.
"""
Comment 1 Danielle Madeley 2010-06-24 03:56:38 UTC
Implementing branch: http://git.collabora.co.uk/?p=user/danni/dbus-glib.git;a=shortlog;h=refs/heads/gvalue-to-gvariant

Tests coming soon.
Comment 2 Danielle Madeley 2010-06-24 05:12:06 UTC
Branch is now updated with tests.
Comment 3 Will Thompson 2010-06-24 05:21:42 UTC
(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?
Comment 4 Danielle Madeley 2010-06-24 07:04:31 UTC
(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.
Comment 5 Danielle Madeley 2010-06-24 21:16:37 UTC
(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.
Comment 6 Will Thompson 2010-07-05 03:18:53 UTC
(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.
Comment 7 Simon McVittie 2010-07-12 09:36:49 UTC
> +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.
Comment 8 Danielle Madeley 2010-07-13 22:51:11 UTC
(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.
Comment 9 Simon McVittie 2010-07-14 02:32:13 UTC
(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.
Comment 10 Danielle Madeley 2010-07-14 02:37:45 UTC
(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.
Comment 11 Will Thompson 2010-08-02 06:02:52 UTC
Looks good to me!