Created attachment 18673 [details] [review] Fix some signedness warnings in the dbus-marshal-basic tests. As discussed in bug 8164, there is an unhealthy mix of signed and unsigned char* variables in dbus that causes a bunch of compiler warnings, which in turn hides the useful compiler warnings. This series of patches fixes all such compiler warnings by changing "unsigned char" to just plain "char" wherever possible. There are a few lingering casts, and this definitely doesn't go all the way to removing unsigned chars everywhere, but it's a start. "make check" passes, both in the default case and with "-funsigned-char" specified in CFLAGS. Note that the files were created with "git format-patch", so should apply easily through "git am"
Created attachment 18674 [details] [review] Replace unsigned char*s with signed char*s in dbus/dbus-marshal-*
Created attachment 18675 [details] [review] Get rid of unsigned char*s in dbus-marshal-validate.c
Created attachment 18676 [details] [review] Fix signed vs unsigned char* warnings in dbus-sha.c
Created attachment 18677 [details] [review] Fix signed vs. unsigned char* warnings in dbus-string-util.c
Hi, at a high level I think these patches are good - right now it's harder to catch warnings in the code due to all the unsigned warnings. However we do have a generally working codebase, some more important outstanding patches and bugs, and not a lot of maintainer time. When I get a bit of spare time I am going to look at some of Scott's patches first and the Nokia uuid patch; if someone else wants to carefully review these patches, that would of course be appreciated.
Havoc seems to agree about the direction of this change. On Bug #17289: > It should be a separate bug for any real discussion, but fwiw on the > signed/unsigned char the desired fix is to never use "unsigned char" imo. This > will be a huge patch unfortunately.
Personally, in projects that I am writing from scratch now, I take the stance of always using "guint8*" when I mean "byte array", and "char*" when I mean "utf8 NUL terminated string". This is roughly what DBus tries to do now, and I agree with it. Also, C has the huge misfeature that the signedness of "char" is unspecified, and this can easily create problems where certain code works on Intel but not ARM and PowerPC. So I disagree with these patches. Either we: 1) Stick with unsigned char and add casts where necessary to char* for interoperation with C stdlib stuff like strchr() (or better, change them to memchr() or the like). 2) Consistently use "signed char" and add casts (but these will be less obvious since we don't really have any developers looking at PowerPC or ARM compilation logs, although I guess we could test with -funsigned-char).
Right, I tried to use unsigned char for byte array and char for UTF-8. The basic issue that came up and made a mess is that I also tried to use DBusString for both of those, I think, so the DBusString API is always wrong for one of them. Maybe another fix would be to have separate "binary" and "utf8" copies of a bunch of the DBusString methods, but I was thinking just giving up on trying to distinguish these two might be easier. I'm not sure distinguishing has all that much value. I could see it either way.
(In reply to comment #8) > Right, I tried to use unsigned char for byte array and char for UTF-8. The > basic issue that came up and made a mess is that I also tried to use DBusString > for both of those, I think, so the DBusString API is always wrong for one of > them. I think it'd make sense to have DBusString consistently take guint8*, since there's only one method on it that has anything to do with text, which is "_dbus_string_validate_utf8".
See also Bug #15522. *** This bug has been marked as a duplicate of bug 15522 ***
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.