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)
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
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)
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)
(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.