Bug 18040

Summary:	tp_list_connection_managers() (and others?) use const on complex types.
Product:	Telepathy	Reporter:	Murray Cumming <murrayc>
Component:	tp-glib	Assignee:	Telepathy bugs list <telepathy-bugs>
Status:	RESOLVED INVALID	QA Contact:	Telepathy bugs list <telepathy-bugs>
Severity:	normal
Priority:	medium
Version:	unspecified
Hardware:	Other
OS:	All
Whiteboard:
i915 platform:		i915 features:

Description Murray Cumming 2008-10-13 03:26:56 UTC

tp_list_connection_managers() takes a TpConnectionManagerListCb callback that provides a list of const TpConnectionManager*.
http://telepathy.freedesktop.org/doc/telepathy-glib/telepathy-glib-connection-manager.html#TpConnectionManagerListCb

But const should never be used in C for complex types. It is only useful for simple types (such as char) or structs of simple types, such as GdkRectangle. Unlike C++, C lacks the mutable keyword, and lacks const and non-const method overloads, so const just gets annoying for complex types. This is the approach used by GTK+.

For instance, this code causes compiler warnings because we use g_object_get() on a const instance. To avoid these, const must be annoyingly casted away almost always.


static void
on_list_connection_managers(TpConnectionManager * const *connection_managers,
                            gsize n_cms, 
                            const GError *error,
                            gpointer user_data,
                            GObject *weak_object)
{
  TpConnectionManager * const *cm_iter = connection_managers;
  for (; *cm_iter != NULL; ++cm_iter)
    {
      const TpConnectionManager *cm = *cm_iter;
   
      gchar *cm_name = NULL;
      g_object_get (G_OBJECT(cm),
        "connection-manager", &cm_name,
        NULL);
  ...


(see also 
http://bugs.freedesktop.org/show_bug.cgi?id=17115
about the general horribleness of the pointers-to-pointers in the API.)

Comment 1 Simon McVittie 2008-10-14 03:10:08 UTC

Er, isn't this basically the same code as examples/client/list-managers.c? Except that the example handles errors correctly, while the code you pasted appears to dereference @cms even if @error is non-NULL, causing a segfault (@cms is documented as "... or %NULL on error".

list-managers.c doesn't bother declaring an equivalent of your variable "cm" (it just uses *iter directly, since there's only one mention of it in the rest of the function) but the following patch to make it resemble your code compiles without warnings too. The problem is that you used "const TpConnectionManager *" where just "TpConnectionManager *" would do.

TpConnectionManager * const * is a const pointer (pointer that you must not free) to TpConnectionManager *, so it is correct to dereference it and assign the result to a TpConnectionManager * variable.

diff --git a/examples/client/list-managers.c b/examples/client/list-managers.c
index 33e8aee..904e8b0 100644
--- a/examples/client/list-managers.c
+++ b/examples/client/list-managers.c
@@ -39,9 +39,10 @@ got_connection_managers (TpConnectionManager * const *cms,
 
       for (iter = cms; *iter != NULL; iter++)
         {
+          TpConnectionManager *cm = *iter;
           gchar *name;
 
-          g_object_get (*iter,
+          g_object_get (cm,
               "connection-manager", &name,
               NULL);

Comment 2 Murray Cumming 2008-10-14 08:21:15 UTC

(In reply to comment #1)
> Er, isn't this basically the same code as examples/client/list-managers.c?
> Except that the example handles errors correctly, while the code you pasted
> appears to dereference @cms even if @error is non-NULL, causing a segfault
> (@cms is documented as "... or %NULL on error".

I left the error checking out on purpose to demonstrate more clearly the problem with const. Of course.
 
> list-managers.c doesn't bother declaring an equivalent of your variable "cm"
> (it just uses *iter directly, since there's only one mention of it in the rest
> of the function) but the following patch to make it resemble your code compiles
> without warnings too. The problem is that you used "const TpConnectionManager
> *" where just "TpConnectionManager *" would do.
> 
> TpConnectionManager * const * is a const pointer (pointer that you must not
> free) to TpConnectionManager *,

I thought that would be 
TpConnectionManager * * const
because you can generally read pointer syntax backwards, but as I've said elsewhere pointer to pointer syntax is horrible and should be avoided. At the least, typedefs should be used to clarify it.

I tried it and you seem to be right. Sorry. Thanks.

Comment 3 Xavier Claessens 2008-10-14 09:04:52 UTC

pointer to pointer can be hidden with a typedef, just like GStrv for gchar**. This is useful for bindings too. For example a binding can generate a list of strings if it sees GStrv, but can't do anything if it is a GList* or a gchar**.

What about that?
typedef TpConnectionManagerArray TpConnectionManager* const*;

Comment 4 Mikhail Zabaluev 2008-10-14 09:14:29 UTC

Why not declare the parameter as
TpConnectionManager * connection_managers []

That's equivalent and removes all confusion.

Comment 5 Murray Cumming 2008-10-14 09:29:22 UTC

Xavier, that doesn't add much (even when we correct your syntax).

I was thinking more of

typedef TpConnectionManager* TpConnectionManagerPtr;
const TpConnectionManagerPtr* connection_managers;

It's not pretty, but pointer-to-pointer never is (a GList would be better), but it is at least more readable.

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.