Bug 17115 - [ABI break] telepathy-glib uses pointers to pointers unnecessarily
Summary: [ABI break] telepathy-glib uses pointers to pointers unnecessarily
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: unspecified
Hardware: Other All
: medium enhancement
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: tp-glib-1.0
  Show dependency treegraph
 
Reported: 2008-08-13 07:13 UTC by Murray Cumming
Modified: 2012-05-07 10:43 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Murray Cumming 2008-08-13 07:13:43 UTC
telepath-glib seem to use Thing** when it really doesn't need to. This makes the API awkward, by requiring extra dereferencing, needing extra checks for NULL.

For instance, the TpConnectionManagerListCb callback provides a TpConnectionManager** parameter instead of just a TpConnectionManager*.

http://telepathy.freedesktop.org/doc/telepathy-glib/telepathy-glib-connection-manager.html#TpConnectionManagerListCb

Also, for instance, TpConnectionManagerProtocol::protocols is a pointer to a pointer, but a simple pointer would be just as good for a NULL-terminated array of TpConnectionManagerProtocol:
http://telepathy.freedesktop.org/doc/telepathy-glib/telepathy-glib-connection-manager.html#TpConnectionManager-struct
Comment 1 Murray Cumming 2008-08-13 07:51:00 UTC
This also encourages people to get confused between pointers to arrays, or arrays of pointers (telepathy-glib uses both), leading to weird memory access errors. I really hate this.
Comment 2 Simon McVittie 2008-08-13 08:09:21 UTC
At the time that TpConnectionManagerListCb was written, the size of a TpConnectionManager was not yet fixed, so we couldn't use a packed array of them. The (TpConnectionManager **) is effectively the contents of a GPtrArray of (TpConnectionManager *), with some attempt at type-safety.

As far as I know, you also can't arrange for g_object_new() to allocate GObjects in an array. So, WONTFIX/CANTFIX.

The gtkdoc for TpConnectionManagerProtocol specifically says that its size is not yet fixed, so again, we can't have an array of TpConnectionManagerProtocol without breaking ABI if/when this size changes. WONTFIX/CANTFIX again.

I'll leave this bug open for the moment as a reminder that a cleaner API + deprecating the old API might be good to have.
Comment 3 Murray Cumming 2008-08-13 08:20:14 UTC
So use a GSList then, like GTK+ would for similar API. Sure, g_object_new() can't allocate more than one object, so call it more than once. 
Comment 4 Murray Cumming 2008-08-17 05:38:05 UTC
(In reply to comment #2)
> At the time that TpConnectionManagerListCb was written, the size of a
> TpConnectionManager was not yet fixed, so we couldn't use a packed array of
> them. The (TpConnectionManager **) is effectively the contents of a GPtrArray
> of (TpConnectionManager *), with some attempt at type-safety.

I also don't see the sense of this. The size of a TpConnectionManager could affect the size of a struct if it had an array of them as a member field, but it's not going to affect the ABI when TpConnectionManager* is used as a function (or callback) parameter. Either way, the application needs to know how big the TpConnectionManager is. Requiring an extra dereference doesn't change that.

And the struct implementations should be private, using the GObject priv system, ideally, so there won't be a problem with base sizes changing.

Comment 5 Will Thompson 2008-10-13 04:09:17 UTC
One advantage of using Foo ** is that you do not have to cast while iterating across the array, and the type of elements of the list is right there in the type signature; when using GSLists, it's easy to get the type wrong.
Comment 6 Simon McVittie 2012-05-07 10:43:33 UTC
tp_list_connection_managers_async() and tp_connection_manager_dup_protocols() supersede the constructs that Murray didn't like, so I think we can call this FIXED.

The superseded things are on their way out too (I have a branch that deprecates them for 0.20, after which I'll delete them from the incompatible 'next' branch which will become 1.0).


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.