[Bug 51356] Fix C warnings in the Telepathy tests library

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=51356

--- Comment #5 from Philip Withnall <bugzilla at tecnocode.co.uk> 2012-07-30 12:35:06 UTC ---
(In reply to comment #4)
> Comment on attachment 64939 [details] [review]
> Fix C warnings in the Telepathy tests library (updated)
> 
> Review of attachment 64939 [details] [review]:
> -----------------------------------------------------------------
> 
> ::: tests/lib/contact-list-manager.c
> @@ +512,4 @@
> >  
> >  static void
> >  status_changed_cb (TpBaseConnection *conn,
> > +                   TpConnectionStatus status,
> 
> Given that the formal signature of the signal is (guint, guint), I would prefer
> not to have this part of the diff. What makes it necessary?

It allows gcc to warn about missing cases in the switch statement (on ‘status’)
which follows. I could change it to be an assignment to a local variable
instead, if you want to preserve the formal signature.

> ::: tests/lib/room-list-chan.c
> @@ +125,4 @@
> >    TpBaseChannelClass *base_class = TP_BASE_CHANNEL_CLASS (klass);
> >    GParamSpec *spec;
> >    static TpDBusPropertiesMixinPropImpl room_list_props[] = {
> > +      { "Server", (gpointer) "server", NULL, },
> 
> Is the const-strings compiler flag actually ever useful? Does it detect bugs,
> or just force us to add pointless casts?

Well, if some code which took in a TpDBusPropertiesMixinPropImpl then modified
the second field, you’d end up modifying a constant string, which this flag
catches.

It has caught legitimate bugs for me in the past (generally when passing a
const string into a function which then modifies it), but it does have a fairly
low signal-to-noise ratio.

> In general more strictness > less strictness, but also, no casts > casts.
> Because C doesn't have C++'s const_cast<>, sprinkling casts throughout your
> source makes it less type-safe than it would otherwise have been. I don't think
> that really helps anyone.

This is true. If possible, a better fix would be to change the data structure
to use const gchar* rather than gpointer for the affected field, but I don’t
think that’s the right fix here, unfortunately.

> If there's a cast here, I would prefer it to be (gchar *) or (char *).

OK. Shall I come up with a new iteration of the patch?

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.
You are the assignee for the bug.