[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 #7 from Philip Withnall <bugzilla at tecnocode.co.uk> 2012-07-30 15:38:46 UTC ---
(In reply to comment #6)
> (In reply to comment #5)
> > It allows gcc to warn about missing cases in the switch statement
> 
> That's not fixing a warning, that's introducing a new warning and then fixing
> it :-P

True. It stems from the first version of this patch, where I added a default
case (as per -Wswitch-default) but didn’t add a case for CONNECTING, causing
things to fail.

> Given that the default behaviour of an unhandled case in a switch is "do
> nothing", and that's entirely appropriate for a change to a status you don't
> recognise, I don't think the current code is wrong.
> 
> (Thought experiment: what would happen if Telepathy 1.0 added a DISCONNECTING
> status, or a NEARLY_THERE status, or something? Answer: it would be reasonable
> for nothing to happen when arriving at that status, the same as is currently
> done for CONNECTING. So "do nothing by default" seems good.)

The advantage I see in -Wswitch-enum is its value when adding new values to
enum types which _should_ be handled everywhere. For example, imagine an ERROR
status, which should cause all clients to return immediately. A do-nothing
default case isn’t going to work for that. The value of -Wswitch-enum is that
it enables the compiler to warn you about every location where you’ve forgotten
to handle the new ERROR value.

> > it does have a fairly
> > low signal-to-noise ratio.
> 
> Yes, that's my objection. I think it's more likely that sprinkling mutable
> casts through the code causes a bug to go undetected than that a string
> constant is unintentionally modified.

I think it’s the other way round. Programmers must explicitly add casts to
disable the warnings where there are false positives, which they should think
about before adding. If the warning flag was disabled, all false negatives
would go unnoticed.

> > If possible, a better fix would be to change the data structure
> > to use const gchar* rather than gpointer for the affected field
> 
> This is an incompatible change (I think) but I would not necessarily object to
> it for Telepathy 1.0. In this case I don't think the field is always a char
> pointer (it's a user_data sort of thing) but gconstpointer would be OK.

I think gpointer → gconstpointer is _not_ an incompatible change. GLib has
certainly done it for a few struct fields recently.

-- 
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.