char signed vs unsigned build failures

RESOLVED FIXED in Thunderbird 36.0

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: stevensn, Assigned: stevensn)

Tracking

unspecified
Thunderbird 36.0
PowerPC
Linux

Thunderbird Tracking Flags

(thunderbird36 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8507548 [details] [diff] [review]
signed_char.diff
Attachment #8507548 - Flags: review?(iann_bugzilla)

Comment 2

4 years ago
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.
Attachment #8507548 - Flags: review?(kent)
(Assignee)

Comment 3

4 years ago
Created attachment 8511568 [details] [diff] [review]
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.
Attachment #8507548 - Attachment is obsolete: true
Attachment #8507548 - Flags: review?(kent)
Attachment #8507548 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

4 years ago
Attachment #8511568 - Flags: review?(kent)

Comment 4

4 years ago
Created attachment 8511590 [details] [diff] [review]
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.
(Assignee)

Comment 5

4 years ago
8511590 builds for me on powerpc

Comment 6

4 years ago
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.
(Assignee)

Comment 7

4 years ago
I am satisfied with the revised patch( 8511590)

Comment 8

4 years ago
Checked in https://hg.mozilla.org/comm-central/rev/273bcc4d6e9b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 36.0

Updated

4 years ago
Attachment #8511568 - Flags: review?(kent)

Updated

4 years ago
Attachment #8511590 - Flags: review+
Assignee: nobody → steve

Updated

4 years ago
Duplicate of this bug: 1103342
Duplicate of this bug: 1109510

Comment 11

4 years ago
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.
Flags: needinfo?(steve)

Comment 12

4 years ago
(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.
(Assignee)

Comment 13

4 years ago
Andreas, 
The patch looks like a good idea to me but you will need to open a new bug for it.
Flags: needinfo?(steve)

Updated

4 years ago
Depends on: 1129048
status-thunderbird36: --- → fixed
You need to log in before you can comment on or make changes to this bug.