Created attachment 64939 [details] [review]
Fix C warnings in the Telepathy tests library (updated)

Updated to apply to current master.

IMO fixing warnings without adding the corresponding -Wfoo is useless because it will regress tomorrow. I don't think any C code ever considered "foo" a const thing. That flag will just never work in the real world (but I agree it should be considered const).

So IMO this just makes part of our code base looks uncommon. I would agree if we decide to respect that across the whole project and force the gcc flags.

Unless tp-glib maintainer has another opinion? jonny?

I can add the corresponding -Wfoo to the build, but I don’t have time to go through and fix all the const warnings in the whole of Telepathy, so that would be fairly pointless. I would be happy to fix the -Wswitch-enum and -Wswitch-default warnings (and add those flags), though. Fixing those is probably more useful.

The point of this patch is to fix the warnings which affect the files copied into libfolks. Since those files aren’t copied across very often, I’d be quite happy to simply fix any new warnings in Telepathy every time we sync the files.

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?

::: 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?

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.

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

(In reply to comment #4)
> Comment on attachment 64939 [details] [review] [review]
> Fix C warnings in the Telepathy tests library (updated)
> 
> Review of attachment 64939 [details] [review] [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?

(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

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

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

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

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

(In reply to comment #7)
> (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.

*Poke*

Abandoning this. It didn’t fix any real bugs, just warning fixes. Not worth the mental overhead of keeping it open given that it’s unlikely to ever get pushed, and there are more important things for review time to go on.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-glib/issues/94.