Version 1.2.1: 1: "dbus-monitor.c", line 94.42: 1506-275 (S) Unexpected text ',' encountered. "," are not allowed as a trailing element in ISO C. gcc lets it by, but its not proper. 2: "dbus-marshal-header.c", line 1429.11: 1506-068 (E) Operation between types "unsigned char*" and "char*" is not allowed. "dbus-marshal-header.c", line 1450.11: 1506-068 (E) Operation between types "const unsigned char*" and "const char*" is not allowed. (first warning) unsigned char *flags_p; flags_p = _dbus_string_get_data_len (&header->data, FLAGS_OFFSET, 1); _dbus_string_get_data_len returns char*, not unsigned char*. 3: "dbus-marshal-byteswap.c", line 167.48: 1506-280 (E) Function argument assignment between types "const char*" and "unsigned char*" is not allowed. _dbus_string_init_const_len expects a const char*, but is being passed a char*. "dbus-marshal-byteswap.c", line 240.51: 1506-280 (E) Function argument assignment between types "unsigned char*" and "char*" is not allowed. byteswap_body_helper expects a unsigned char*, but _dbus_string_get_data_len returns char*. There are plenty more of these types of unsigned/const char* typecasting issues. I can post them all if someone is interested in looking at them.
1. It isn't part of our style guides so I will take that out 2. known issue, if someone was willing to send a correct patch (big work, little payoff) we will apply it but otherwise it is harmless since it is internal code and not part of the public API 3. again, a lack of coordination between internal API during the early stages of development. Patches just using casts to fix these issue will not be accepted. We would accept correct patches which fix the issue by imposing a standard for how we type with casting used only when necessary.
Created attachment 86703 [details] [review] [PATCH] Use char instead of unsigned char to fix cast warning Still appliable on master branch though it was reported against 1.2.X, since it's a very old bug, I'm eagering to close it.
> Since there is no mathematical operation based on char or unsigned char, > the only mathematical operations are cast to int, so it's safe. Be careful with this sort of reasoning: ((int) (char) x) != ((int) (unsigned char)) x if ((unsigned char) x) is in the range 128-255. (In reply to comment #1) > Patches just using casts to fix these issue will not be accepted. In general I'm inclined to agree, unless there are actually-real-world-useful compilers that won't do a signedness-changing cast.
(In reply to comment #3) > > Since there is no mathematical operation based on char or unsigned char, > > the only mathematical operations are cast to int, so it's safe. > > Be careful with this sort of reasoning: ((int) (char) x) != ((int) (unsigned > char)) x if ((unsigned char) x) is in the range 128-255. Yes, it's true. I think the patch is safe since I think there is only address used but the value. > > (In reply to comment #1) > > Patches just using casts to fix these issue will not be accepted. > > In general I'm inclined to agree, unless there are > actually-real-world-useful compilers that won't do a signedness-changing > cast.
Comment on attachment 86703 [details] [review] [PATCH] Use char instead of unsigned char to fix cast warning Review of attachment 86703 [details] [review]: ----------------------------------------------------------------- > Since there is no mathematical operation based on char or unsigned char This is not true. Perhaps you're only directly changing the types of pointers, but as soon as you dereference a pointer of changed type via *p or p[0], you get a changed value. ::: dbus/dbus-marshal-byteswap.c @@ +144,2 @@ > > sig_len = *p; Consider a signature of maximum length, 255 bytes. In the wire protocol that's one byte with value 0xFF. sig_len is an int, *p is a char, sig_len = -1 (assuming two's complement). I'm sure there's more like this - this is just the first example I spotted. @@ -144,2 @@ > > sig_len = *p; Previously: sig_len is a uint32, *p is an unsigned char, sig_len = 255, correct value.
If someone wants to address this fully, the best way to fix this sort of thing without sprinkling casts through the code would probably be inline functions like this in dbus-string.h: static inline unsigned char * _dbus_string_get_udata_len (DBusString *self, int offset, int len) { char *signed = _dbus_string_get_data_len (self, offset, len); return (unsigned char *) signed; } It's completely obvious that the cast there is only changing signedness, not constness or size, which is good. However, this would still be a lot of code churn for remarkably little benefit, so I would be reluctant to take patches for this unless someone can name a used-in-the-real-world compiler that needs this distinction, and a good reason why it isn't acceptable to use gcc or clang instead.
*** Bug 17433 has been marked as a duplicate of this bug. ***
See also: discussion on Bug #17433.
If we do something like this, we should enable -Wpointer-sign (-Werror=pointer-sign in development builds) via the TP_COMPILER_WARNINGS macro in configure.ac, so the bug doesn't come back.
(In reply to Simon McVittie from comment #6) > If someone wants to address this fully, the best way to fix this sort of > thing without sprinkling casts through the code would probably be inline > functions like this in dbus-string.h: > > static inline unsigned char * > _dbus_string_get_udata_len (DBusString *self, > int offset, > int len) > { > char *signed = _dbus_string_get_data_len (self, offset, len); > > return (unsigned char *) signed; > } > > It's completely obvious that the cast there is only changing signedness, not > constness or size, which is good. > > However, this would still be a lot of code churn for remarkably little > benefit, so I would be reluctant to take patches for this unless someone can > name a used-in-the-real-world compiler that needs this distinction, and a > good reason why it isn't acceptable to use gcc or clang instead. The benefit is that developers can concentrate on real-world-programming instead to think any time on every compile if-this-cast-may-be-or-not-an-issue as documentated at https://bugs.freedesktop.org/show_bug.cgi?id=89284#c18 "The implicit conversion of this_time from int or ssize_t to size_t in line 94 can be proved to be safe, because we know this_time is non-negative - so I'm surprised gcc is issuing this warning. Every non-negative int is in-range for size_t on Windows (sizeof(int) is 4 on Windows, and sizeof(size_t) is 4 or 8), and every non-negative ssize_t is in-range for size_t on Unix (sizeof(ssize_t) == sizeof(size_t) by definition)."
*** This bug has been marked as a duplicate of bug 93069 ***
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.