[PATCH] D-Bus 1.0.x Warning Fix Ups

[Havoc Pennington]

Hi,

Doug Goldstein wrote:
> Basically, if you turn on any set of real warnings D-Bus fails to
> compile and has a lot of warnings. 

I don't think that's quite fair - dbus builds by default with both -Wall 
and a bunch of additional warnings not in -Wall. My basic guideline is 
to include any warning that might detect a real bug and won't produce a 
lot of false alarms. I don't like putting in bogus or weird code just to 
hide warnings, so tend to avoid warnings that will be extensively 
triggered when the code is already clear and correct. But it may still 
be worth scanning the output of a build with such overzealous warnings 
from time to time.

There are a pile of warnings right now about signed/unsigned char. I've 
been reluctant to fix these because it will be a giant patch, plus I 
can't really decide how to fix them. (casts are not a good answer)

Anyway, some of these fixes look good, thanks. Any warning that we 
commit patches to clean up, should also go into configure.in so it's 
included in everyone's build.

For warnings with lots of false positives, we could add a "configure 
--overzealous-compile-warnings" or something to make it easy to scan 
through those warnings occasionally.

Another note, please don't turn on -ansi -pedantic (I don't think you 
are, but sometimes people report issues with these). The first is bogus 
because dbus knows when it is and isn't using gcc, and does use gcc 
features when it is; the second is aptly-named. I think the gcc manual 
may even describe these as bogus.

> -      DBusAddressEntry *entry;
> +      DBusAddressEntry *item;

Variable shadowing I think is worth fixing, it's pretty confusing. An 
even better fix though might be to change this to a more meaningful 
name, so the two different names are hard to confuse. (a litmus test 
might be, if you swapped the name of this variable and the shadowed 
variable, would it still make sense? if so then they don't really have 
meaningful names)

> --- dbus/dbus-auth.c	2006-09-05 20:14:06.000000000 -0400
> +++ dbus/dbus-auth.c.new	2006-12-03 23:46:17.000000000 -0500
> @@ -752,6 +752,8 @@
>    const DBusString *username;
>    dbus_bool_t retval;
>  
> +  (void)auth;

I'm not a fan of this... I don't recall a lot of bugs in many years of C 
programming that "unused parameter" would detect, and there are lots of 
legitimate reasons to have unused parameters.

If we do add fixes like this, I would always comment it like
  (void)auth; /* quiet compiler warnings */

but if a warning option mostly generates code like this instead of 
finding potential bugs or bad code, I think we should not put these 
fixes in.

>      handle_client_shutdown_cookie_sha1_mech },
> -  { NULL, NULL }
> +  { NULL, NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL, NULL}

This is a good change also imo (the first version is correct, but less 
clear)

> -#define _dbus_string_get_length(s) (((DBusString*)(s))->dummy2)
> +#define _dbus_string_get_length(s) (((const DBusString*)(s))->dummy2)

Seems reasonable.

>  #define _dbus_string_set_byte(s, i, b) ((((unsigned char*)(((DBusString*)(s))->dummy1))[(i)]) = (unsigned char) (b))
>  #define _dbus_string_get_byte(s, i) (((const unsigned char*)(((DBusString*)(s))->dummy1))[(i)])
> -#define _dbus_string_get_const_data(s) ((const char*)(((DBusString*)(s))->dummy1))
> -#define _dbus_string_get_const_data_len(s,start,len) (((const char*)(((DBusString*)(s))->dummy1)) + (start))
> +#define _dbus_string_get_const_data(s) ((const char*)(((const DBusString*)(s))->dummy1))
> +#define _dbus_string_get_const_data_len(s,start,len) (((const char*)(((const DBusString*)(s))->dummy1)) + (start))

Also looks good.

Thanks,
Havoc