1085151 - char signed vs unsigned build failures

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Steve Singer (:stevensn)]

seamonkey fails to build on powerpc with the following type of errors

comm-central/mailnews/base/search/src/nsMsgFilterList.cpp:493:22: error: comparison is always true due to limited range of data type [-Werror=type-limits]
   while (curChar != -1);

'char' on powerpc is unsigned.

Comment 1

[Steve Singer (:stevensn)]

Attachment: signed_char.diff

Comment 2

[:aceman]

Comment on attachment 8507548 [details] [diff] [review]
signed_char.diff

Don't you also need to change the return value of ReadChar() as that is the one that may return -1 ? Or maybe we should convert all to unsigned char and have 255 be the special value.

Comment 3

[Steve Singer (:stevensn)]

Attachment: signed_char.diff

Updated version of the patch that changes the return value of ReadChar() and 
catches a few other char usages in the file.

Comment 4

[Kent James (:rkent)]

Attachment: Replace magic -1 with define

What would you think of the approach? Does this work with powerpc?

Because nsIInputStream.read is expecting a charPtr (which is char *) I hate to switch to signed or unsigned char to make a comparison compile. I would rather just do a proper comparison. Magic numbers like -1 are generally discouraged, so in the process I cleaned that up with a macro.

Comment 5

[Steve Singer (:stevensn)]

8511590 builds for me on powerpc

Comment 6

[Kent James (:rkent)]

So are you satisfied with the revised patch? This is really my suggested changes to your patch, so you need to also be OK with it.

Comment 7

[Steve Singer (:stevensn)]

I am satisfied with the revised patch( 8511590)

Comment 8

[Kent James (:rkent)]

Checked in https://hg.mozilla.org/comm-central/rev/273bcc4d6e9b

Comment 11

[Andreas Schwab]

Attachment: Use out-of-bounds value as EOF marker.

This is the wrong approach. 0xFF is a valid character (ÿ in ISO-8869-1, for example), and any attempt to use a character as an EOF marker is doomed to fail.

Comment 12

[Kent James (:rkent)]

(In reply to Andreas Schwab from comment #11)
> Created attachment 8544581 [details] [diff] [review]
> Use out-of-bounds value as EOF marker.
> 
> This is the wrong approach. 0xFF is a valid character (ÿ in ISO-8869-1, for
> example), and any attempt to use a character as an EOF marker is doomed to
> fail.

This bug was about fixing a build failure, and did not introduce the concept of using 0xFF as an EOF marker. Please feel free to file a new bug where the issue of possible problems with using EOF in this manner can be discussed directly.

Comment 13

[Steve Singer (:stevensn)]

Andreas, 
The patch looks like a good idea to me but you will need to open a new bug for it.