As a compliment to tp_asv_get_*(), I'd like to propose the addition of a: GHashTable *tp_asv_new (const char *first_key, ...) Which calls: GHashTable *asv = g_hash_table_new_full ( g_str_hash, g_str_equal, NULL, (GDestroyNotify) tp_g_value_slice_free); And then takes a NULL-terminated vararg list, as follows: tp_asv_new ("answer", G_TYPE_INT, 42, "question", G_TYPE_STR, "we just don't know", NULL); Plus a full set of `void tp_asv_set_TYPE(GHashTable *asv, const char *key, TYPE v)` functions to complement the existing tp_asv_get_TYPE functions e.g. void tp_asv_set_string (GHashTable *asv, const char *key, char *value) { g_hash_table_insert (asv, key, tp_g_value_slice_new_string (value)); } This is effectively a rehash of bug #18220, but I think it would significantly help to improve the readability of a{sv} maps and thus is worth having. e.g. it drops this map setup for RequestConnection: GHashTable *parameters = g_hash_table_new_full ( g_str_hash, g_str_equal, NULL, (GDestroyNotify) tp_g_value_slice_free); g_hash_table_insert (parameters, "account", tp_g_value_slice_new_string ("davyd")); g_hash_table_insert (parameters, "password", tp_g_value_slice_new_string ("sup")); Down to 3-4 nicely wrapped lines: GHashTable *parameters = tp_asv_new ( "account", G_TYPE_STRING, "davyd", "password", G_TYPE_STRING, "sup", NULL);
http://git.collabora.co.uk/?p=user/davyd/telepathy-glib-davyd.git;a=shortlog;h=refs/heads/bug-20942
Coding style: Telepathy style for wrapping function calls is: foo_get_bar (very, long, argument, list, ..., four_space_indent); I note that http://git.collabora.co.uk/?p=user/davyd/telepathy-glib-davyd.git;a=commitdiff;h=904a2891a96d4ac56128cf9766ac7d9386e53481 "fixes" the indentation still to be wrong. :) From the perspective of helping people not get this wrong, I'd replace: "a #GHashTable for storing a{sv} maps" with: "a #GHashTable created with tp_asv_new()" If people want to make their own hash table, they can read the tp_asv_new() documentation and discover what they must do.
Ok. I should have unnoobed the indentation now. Added some missing symbols, and a tp_asv_dump() for debugging. Also added the symbols to the reference docs (which apparently has to be done manually -- crazy).
Some review comments: Each tp_asv_set_* that takes a pointer should have tp_asv_take_* and tp_asv_set_static_* variants. The _take_ ones are useful when you're allocating something on the spot, and the _set_static_ ones are useful when you have some immutable static data in your C file. I think tp_asv_set_bytes() should be slightly inconsistent by taking a length and a gconstpointer, like tp_g_value_slice_new_bytes() does, so it can be used for non-GArray sources of bytes (since the byte array is being copied anyway, there's no requirement that they come from a genuine GArray). If what you're copying *is* in fact a GArray, then tp_asv_set_bytes (asv, "BinaryBlob", array->len, array->data) is just as easy. (You might have missed some of the boxed GValue slice allocators, because they're in dbus.[ch] rather than util.[ch] - the idea is that anything using GLib can benefit from sliced GValues, but the boxed ones like object path and bytes are specific to dbus-glib). tp_asv_new() should have the G_GNUC_NULL_TERMINATED annotation.
Should all of the tp_g_value_slice_new() functions be documented in the same place? At the moment they're spread across two sections, which as you say, can cause you to miss them.
Ok branch updated per feedback.
Merged.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.