Bug 15522

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