Atm tp-glib uses both GList and GPtrArray when exposing collection of objects. I think we should prefer the latter: - Can be reffed so user don't have to copy - Easier memory management using the free_func - Take less memory
I'm a bit concerned about diverging from what GIO is establishing as the convention. In GIO, these return a (transfer full) GList: g_app_info_get_all g_content_types_get_registered g_dbus_object_get_interfaces g_dbus_object_manager_get_objects g_drive_get_volumes g_file_enumerator_next_files_finish g_io_modules_load_all_in_directory g_resolver_lookup_by_name g_resolver_lookup_by_name_finish g_resolver_lookup_service g_resolver_lookup_service_finish g_tls_certificate_list_new_from_file g_tls_client_connection_get_accepted_cas g_tls_database_lookup_certificates_issued_by g_tls_database_lookup_certificates_issued_by_finish g_unix_mount_points_get g_unix_mounts_get g_volume_monitor_get_connected_drives g_volume_monitor_get_volumes and these return a (transfer none) GList (which might be considered a bug, seeing how overwhelmingly (transfer full) the rest of GIO is): g_emblemed_icon_get_emblems g_io_extension_point_get_extensions This also makes our current use of (transfer container) in, for instance, tp_account_manager_get_valid_accounts() look like an API wart; maybe we should change them all to (transfer full) for 1.0? I'm also not sure how you annotate a GPtrArray that has a free-function set. I think it's probably (transfer container), since the caller only needs to free the array? But I could be wrong... GPtrArray has the advantage that you can ref it easily, and the disadvantage that if a caller mutates the returned GPtrArray and it was a ref to one kept internally by our object, "it changes for everyone"; similarly, if our object updates its internal cache in response to D-Bus signals, anyone still holding a ref may find that their copy has changed.
(In reply to comment #1) > In GIO, [... most things ...] return a (transfer full) GList I think our (transfer container) convention may have come from Gtk, which seems to return mostly (transfer container), quite a lot of (transfer full), and occasionally (transfer none) just to confuse you. Gtk doesn't return any GPtrArrays either. If we ever want to be thread-safe, we basically have to return (transfer full), or a freshly constructed GPtrArray (not just a ref to our internal cache) with a free-function and (transfer container).
Before getting much further with next, we should decide: ---- 1. What is our preferred transfer mode for single objects? The options are none and full. We should choose one to use whenever the name of a method doesn't specifically indicate it ("dup", "borrow"). In favour of none: * it's the easiest "C binding" In favour of full: * you have to use it anyway if you ever want to become thread-safe * you have to use it anyway if you compute the value on-demand Whichever one we choose, we can use "dup" or "borrow" to make it explicit when we're using the other one. Additionally, I think whenever we have a "dup" or "borrow" without a corresponding "get", in Telepathy 1.0 we should use "Rename to:" to rename it to get_foo for introspective bindings. (This is an introspection API break.) Current practice in tp-glib: "get" mostly means (transfer none), but a lot of things are only available as "dup". Sometimes functions with transfer none are called "borrow" anyway. My opinion: we should bite the bullet and make "get" mean (transfer full). We can have parallel "borrow" functions as "C bindings" where it makes sense, and use (skip) to omit them from introspection. ---- 2. What is our preferred transfer mode for plural objects? Options are none, container and full. none and full have the same advantages as for singular objects. full is also popular in GIO. In favour of container: it's popular (although by no means the only thing used) in Gtk. Current practice in tp-glib: "get" means (transfer container), e.g. in TpAccount. This may have been Jonny's idea, or he might have just copied current practice from Empathy. My opinion: I think we should make "get" mean (transfer full), and have parallel "borrow" functions if we really need them. ---- 3. What is our preferred container for plural getters? The options are basically GList and GPtrArray, with a dishonourable mention for GSList if you like micro-optimizations. In favour of GList: * everything else uses it * returning a non-refcounted type guarantees that any changes made by the object are not visible to callers with an old copy, and vice versa * never needs to realloc * the empty list takes no memory * short lists take less memory than anything else, except GSList * forward and reverse iteration is O(n) * you can do O(n) list building via a GQueue or the prepend-and-reverse pattern In favour of GPtrArray: * you can refcount it (but that might not be such a good idea) * you can set a free_func for safe use of (transfer container) * large arrays use around 1/3 of the memory of GList * reverse iteration still works * building is trivial and O(n) * iteration is trivial and O(n) * most operations are O(1) unless you need to realloc In favour of GSList: * returning a non-refcounted blah blah blah (... same as GList) * uses around 2/3 of the memory of GList * never needs to realloc * the empty list takes no memory * short lists take less memory than anything else * forward iteration is O(n) (but reverse iteration is expensive) Current practice in tp-glib: GList, except where we use a C array with a length My opinion: let's standardize on GList for least-astonishment.
(In reply to comment #3) > 1. What is our preferred transfer mode for single objects? [...] > Additionally, I think whenever we have a "dup" or "borrow" without a > corresponding "get", in Telepathy 1.0 we should use "Rename to:" to rename it > to get_foo for introspective bindings. (This is an introspection API break.) Interesting, I didn't know about "Rename to:". > My opinion: we should bite the bullet and make "get" mean (transfer full). We > can have parallel "borrow" functions as "C bindings" where it makes sense, and > use (skip) to omit them from introspection. Agreed. > 2. What is our preferred transfer mode for plural objects? [...] > Current practice in tp-glib: "get" means (transfer container), e.g. in > TpAccount. This may have been Jonny's idea, or he might have just copied > current practice from Empathy. Good question, I can't remember how things went down any more. > My opinion: I think we should make "get" mean (transfer full), and have > parallel "borrow" functions if we really need them. Agreed. I also think GIO is a better library to copy than Gtk. > 3. What is our preferred container for plural getters? [...] > My opinion: let's standardize on GList for least-astonishment. Agreed again. GPtrArray is a hassle and everyone knows exactly how linked lists work...hopefully. It's a shame we don't have a type-safe container we can use.
I personally like (transfer none) for direct access of object's internal data, like a lot of getters does. But I agree it is a bit dangerous since we can't really return "const GObject" or "const GList*" or things like that. From my personal experience, I've (almost) never seen a getter call followed by a modification of the returned value. It's always to iterate over it, like foreach (account in am.get_valid_accounts()){}. otoh, refcounted structs are cheap to dup, strings are safe to return as const, and collections are always small (to the exception of tp_connection_dup_contact_list). and probably GList is cheap to deepcopy with slice allocator… I still have that crazy idea that stuff like "return g_object_unref_in_idle(obj)" would be useful to let a chance to caller to take its ref while not introducing a leak if he does not care about return value. And to finish, if we go with GList, please make it (transfer full/none), but (transfer container) is just confusing IMO.
Just wanted to mention that borrow is pretty much what we expect from a get in GLib. I found that our very sporadic use of borrow is more confusing then helpful. It's only in GStreamer that get is a transfer full, I guess this has two reasons, first GStreamer was single threaded and not taking a ref is generally not thread safe.
(In reply to comment #6) > Just wanted to mention that borrow is pretty much what we expect from a get in > GLib. I don't think it's actually as clear-cut as that: even in core libraries like GIO, "get" often means "copy" (usually means copy, in GIO's case). In GLib/GObject it often means "don't copy", e.g. g_value_get_object(), except for when it doesn't, e.g. g_object_get()). Gtk is particularly inconsistent about what "get" means. > I found that our very sporadic use of borrow is more confusing then > helpful. We should settle on a meaning for "get"; if it means "copy", only use "get" and "borrow" (or would you prefer "peek", maybe?) in APIs, and if it means "don't copy", only use "get" and "dup" in APIs. Jonny seemed to agree that our standard in Telepathy 1.0 should mimic what most of GIO does: get means transfer full, and the standard container is GList. (In reply to comment #6) > It's only in GStreamer that get is a transfer full ... and GIO, at least for containers (almost entirely consistently, Comment #1). > not taking a ref is generally not thread safe. I think this is a compelling argument for our default being transfer full: if we want to allow telepathy-glib to ever be thread-safe (even if it remains non-thread-safe for now), we have to copy anything that could otherwise be modified or destroyed concurrently with it being used.
(In reply to comment #5) > I still have that crazy idea that stuff like "return > g_object_unref_in_idle(obj)" would be useful to let a chance to caller to take > its ref while not introducing a leak if he does not care about return value. In whose main context do you idle? The caller's thread-default main context? (In case it isn't obvious: I think this is too subtle.)
As no major lib seems to be consistent with itself, we should just decide what makes most sense and stick to it. I vote for _get() being (transfer none) and _dup() being (transfer full). I agree that _borrow() is confusing/ugly, let's just use _get() instead.
It seems that we essentially have two proposals for next. Jonny and I think the policy should be * get means copy * borrow (or peek or something) means don't copy * other verbs mean copy * dup is not a thing * lists are GList with (transfer full) while Guillaume and Xavier think the policy should be: * get means don't copy * borrow/peek is not a thing * dup means copy * other verbs mean (... what? current practice is usually copy) * functions returning a list are always dup, and return GPtrArray with (transfer container) and a free-function 0.19 is similar to the latter, mostly from inertia. We're generally only adding dup, not get, for new APIs, because this is the only thing that can possibly be thread-safe, and also the only way we can (preserve the ability to) return a newly-constructed thing instead of random internal state. Keeping these called "dup_thing" means our g-i bindings are a bit rubbish (Python authors have to call dup_thing() even though they shouldn't have to care about the distinction), because "Rename to" currently only works if the "overwritten" thing exists.
I think we'll decide that we won't decide... This could really be just left undefined and decided on a case-by-case. At least now that we have annotations, we are sure that the transfer method is well documented. IMO the container isn't really important, we can live we inconsistencies, and I'm clearly too lazy to fix existing code in either direction. The most important is to keep related APIs convenient. For example tp_connection_upgrade_contacts_async() is taking n_contacts + c-array, so it makes sense for tp_connection_dup_contact_list() to return a GPtrArray instead of a GList. What I'm more concerned about is the get VS dup. I think getters generally does not ref/copy the return value if it is in the internal format. If it is not the same format, then a _dup_ naming makes it explicit, which is IMO a good thing. I don't think we want to duplicate memory every time we get a value. Except for the transfer-container corner case, I think the rule of get VS dup is already well respected in tp-glib API. In the end, I think the most important is to avoid subtle changes in our high-level API. Changing a _get_ to suddenly return a ref is going to be make app porting much more complex. So I'm open to renaming APIs from _get_ to _borrow_ or _dup_ but NOT changing the behaviour of an existing _get_. I would like to keep in mind that tp-glib-1.0 is not the perfect API as it always should have been. But rather a better API given the historical constrain we have. Let's not make it a long and painful porting. It has already be long enough.
(In reply to comment #11) > I think we'll decide that we won't decide... No, I do not want _get_ to mean different things in different modules. It's bad enough that telepathy-glib is inconsistent with GIO and GLib is inconsistent with itself. > IMO the container isn't really important, we can live we inconsistencies That's reasonable. > What I'm more concerned about is the get VS dup. I think getters generally does > not ref/copy the return value if it is in the internal format. They usually do in GIO, and they have to if you ever want to be thread-safe. > In the end, I think the most important is to avoid subtle changes in our > high-level API. Changing a _get_ to suddenly return a ref is going to be make > app porting much more complex. This is a good argument in favour of what I called the Guillaume/Xavier proposal in Comment #10. (In reply to comment #5) > And to finish, if we go with GList, please make it (transfer full/none), but > (transfer container) is just confusing IMO. Let's supersede our existing GList (transfer container) APIs with a "dup" version (dup_valid_accounts() etc.)? I think there are only a couple.
So from a pragmatic POV: - We have 867 _get_. AFAIK they are all[1] returning the internal pointer/value. That's too much to change. - We have 9 _borrow_. That's small enough to consider renaming them to _get_. - We have 34 _dup_. I would consider that too much to change. [1] With the exception of some transfer-container: * If the structure is a GPtrArray (or anything that knows how to free its elements) I would consider them as dup (AFAIK that's already the case, see tp_connection_dup_contact_list). * GList is really the special case here. They are often _get_ (e.g. tp_account_manager_get_valid_accounts). 1) I surely do NOT want to change them to not dup the glist anymore, that would make crashes in apps badly ported. 2) I would be in favor of making them transfer full and rename to _dup_. 3) I'm ok with just living with that and not change them at all.
(In reply to comment #13) > So from a pragmatic POV: > > - We have 867 _get_. AFAIK they are all[1] returning the internal > pointer/value. That's too much to change. Yes, I think you may be right. I'd have preferred to be consistent with GIO, but as long as we're consistent within tp-glib (1.0), and assuming Jonny doesn't want to veto it, I'm OK with being consistent with tp-glib 0.x instead. It's certainly the lower-risk route. > - We have 9 _borrow_. That's small enough to consider renaming them to _get_. If we do the "add in 0.19, deprecate, remove in next" dance, this can be pretty unintrusive. > - We have 34 _dup_. I would consider that too much to change. Yeah. In some cases we genuinely do need to copy the pointer, and in some cases it gives us flexibility to change the internals. I think we should err on the side of using _dup_ in new APIs, though: it would be nice if telepathy-glib could become thread-safe one day, after we finally eradicate dbus-glib. > * If the structure is a GPtrArray (or anything that knows how to free its > elements) I would consider them as dup (AFAIK that's already the case, see > tp_connection_dup_contact_list). These have to be annotated as (transfer container) but I think naming them _dup_ is fine, yes. > * GList is really the special case here. They are often _get_ (e.g. > tp_account_manager_get_valid_accounts). 1) I surely do NOT want to change them > to not dup the glist anymore, that would make crashes in apps badly ported. Agreed. > 2) > I would be in favor of making them transfer full and rename to _dup_. For the Account GList ones, let's add (transfer full) _dup_ in 0.19, then remove (transfer container) _get_ in 1.0.
There are 7 functions returning GList with "transfer container": tp_account_manager_get_valid_accounts() tp_base_client_get_pending_requests() tp_base_client_get_handled_channels() tp_connection_get_contact_info_supported_fields() tp_contact_search_result_get_fields() tp_contact_get_contact_info() tp_text_channel_get_pending_messages() Note that I've grepped "transfer container" so probably missing not/wrongly annotated CM-side APIs.
Here is already a branch to remove _borrow_: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=transfer
(In reply to comment #15) > There are 7 functions returning GList with "transfer container": > > tp_account_manager_get_valid_accounts() > tp_base_client_get_pending_requests() > tp_base_client_get_handled_channels() > tp_connection_get_contact_info_supported_fields() > tp_contact_search_result_get_fields() > tp_contact_get_contact_info() > tp_text_channel_get_pending_messages() > > Note that I've grepped "transfer container" so probably missing not/wrongly > annotated CM-side APIs. Made a more extensive audit of our code and I confirm those are the only getters that returns GList with transfer-container. Note that I've found some getters that returns GList with transfer-none: tp_base_call_channel_get_contents tp_base_call_content_get_streams tp_base_media_call_stream_get_endpoints So current situation isn't consistent at all. So I'll prepare patch to deprecate transfer-container getters and add corresponding dup with transfer-full. Any objection?
oh, and to add to the policy: _finish() functions dups the return values and out args. I remember having seen an exception to that rule, but couldn't find which function anymore. Could be in another codebase (empathy?).
(In reply to comment #16) > Here is already a branch to remove _borrow_: > http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=transfer + * tp_channel_dispatch_operation_get_channels: (skip) + * @self: a #TpChannelDispatchOperation + * + * Returns a #GPtrArray containing the #TpChannel of this + * ChannelDispatchOperation. + * The returned array and its #TpChannel are only valid while @self is + * valid - copy array and reference channels with g_object_ref() if needed. They are actually only valid until the main loop is re-entered, because channels can be lost. As a result, I would really prefer this one to become some sort of _dup_ to "disconnect" it from the internal representation. + * tp_channel_get_immutable_properties: ... + * The returned hash table should not be altered, and is not necessarily + * valid after the main loop is next re-entered. Copy it with This one has a lot of caveats: * not usefully introspectable * tied to dbus-glib * caller must not alter it * channel reserves the right to alter it * copying it in a safe way (not just ref) involves g_boxed_copy Can't we make the new recommended version return a (transfer full) ref to a GVariant instead? (e.g. call dbus_g_value_build_g_variant() followed by g_variant_ref_sink() - but I think we have a helper for this already)
(In reply to comment #19) > oh, and to add to the policy: _finish() functions dups the return values and > out args. I remember having seen an exception to that rule, but couldn't find > which function anymore. Could be in another codebase (empathy?). I endorse this proposal. It's consistent with GIO. If the async method would otherwise have been called get_thing_async(), I've generally been calling it dup_thing_async() to make this even more obvious.
(In reply to comment #16) > Here is already a branch to remove _borrow_: > http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=transfer Continuing my review. General comment: when what we're doing conceptually is renaming a function + providing backwards compat, I generally prefer the diff to look as though that's what we're doing, rather than introducing a new function with the new name and a copy of the old implementation: /** - * borrow_thing: + * get_thing: * * blah blah blah do things */ gchar * -borrow_thing (...) +get_thing (...) { return implementation (); } + +/** + * borrow_thing: + * + * An old name for get_thing(). + * + * Deprecated: blah blah blah + */ + gchar * + borrow_thing (...) + { + return get_thing (...); + } In other words, I like the way you did tp_proxy_get_interface_by_id(). When the implementation is as trivial as "return self->priv->thing" it doesn't particularly matter, though. ---------------------------- -/* FIXME: in Telepathy 1.0, rename to tp_protocol_get_param */ /** * tp_protocol_dup_param: * @self: a protocol @@ -939,6 +938,7 @@ G_GNUC_END_IGNORE_DEPRECATIONS * or %NULL if not supported. Free with tp_connection_manager_param_free() * * Since: 0.17.6 + * Deprecated: Since 0.UNRELEASED. Use tp_protocol_get_param() instead. */ No, please don't deprecate this one: just remove the comment. dup_param() is useful in its own right, because get_param() returns something that loses validity when the TpProtocol is freed. * a list of #TpConnectionManagerParam structures, owned by the caller * * Since: 0.17.6 + * Deprecated: Since 0.UNRELEASED. Use tp_protocol_get_param() instead. */ GList * tp_protocol_dup_params (TpProtocol *self) Er, why are you deprecating this? If you want to list all the parameters (e.g. for Empathy's fallback account creation UI), tp_protocol_dup_params() is nicer than tp_protocol_dup_param_names() followed by repeated tp_protocol_(get|dup)_param(). +DBusGProxy * +tp_proxy_get_interface_by_id (TpProxy *self, + GQuark iface, + GError **error) +{ The indentation here is weird (although copying from cgit to here destroyed the indentation anyway). +++ b/tools/glib-client-gen.py ... - self.b(' iface = tp_proxy_borrow_interface_by_id (') + self.b(' iface = tp_proxy_get_interface_by_id (') glib-client-gen is meant to generate backwards-compatible code by default, so that it's safe to drop it into an old project and not bump the telepathy-glib dependency. I would prefer: if self.tp_proxy_api >= (0, 19, 9): self.b(' iface = tp_proxy_get_interface_by_id (') else: self.b(' iface = tp_proxy_borrow_interface_by_id (') and patching each Makefile.am that calls it to use --tp-proxy-api=0.19.9 (or whatever the next version of telepathy-glib is going to be - I've lost track). Or, if you don't think this is actually useful, I would accept a patch that deleted the tp_proxy_api tests and always assumed the latest.
New policy has been documented here: http://telepathy.freedesktop.org/wiki/Style/TelepathyGLib
Branch updated
Yeah, looks good. I wondered whether I should be objecting to the addition of new functions that return a GObject without reffing it and preferring _dup_ (thinking mainly of TpChannelDispatchOperation's account and connection), but let's cross that bridge when we're a little closer to being thread-safe.
(In reply to comment #25) > Yeah, looks good. Cool, thanks. > I wondered whether I should be objecting to the addition of new functions that > return a GObject without reffing it and preferring _dup_ (thinking mainly of > TpChannelDispatchOperation's account and connection), but let's cross that > bridge when we're a little closer to being thread-safe. We cannot be even close to threadsafe until we port to gdbus, which will be another API break anyway.
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.