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

[Doug Goldstein]

Havoc Pennington wrote:
> 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.

I'm not bashing it at all. I think you took it the wrong way. I just
cranked up the warning level to what I consider a real set. But my
reality of real is a bit skewed since I work in the financial industry
and we consistently use nearly 3 lines of warnings. But you're right,
they are extreme for most users.


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

That would be a good idea. I can post my warnings and you can cull it
down to an agreeable list.

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

I'll have to double check but I didn't think it was last time I checked.

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

I agree. I didn't really come up with a good name either.

> 
>> --- 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 */

It's one of the warnings I'm not that much of a fan of fixing TBH. But I
hit them all. This was the most notable one that I assumed would be nixed.

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


-- 
Doug Goldstein <cardoe at gentoo.org>
http://dev.gentoo.org/~cardoe/

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 252 bytes
Desc: OpenPGP digital signature
Url : http://lists.freedesktop.org/archives/dbus/attachments/20061204/c1fdaa73/signature.pgp