Closed Bug 1085151 Opened 7 years ago Closed 7 years ago

char signed vs unsigned build failures

Categories

(MailNews Core :: Filters, defect)

PowerPC
Linux
defect
Not set
normal

Tracking

(thunderbird36 fixed)

RESOLVED FIXED
Thunderbird 36.0
Tracking Status
thunderbird36 --- fixed

People

(Reporter: stevensn, Assigned: stevensn)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Attached patch signed_char.diff (obsolete) — Splinter Review
Attachment #8507548 - Flags: review?(iann_bugzilla)
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)
Attached patch signed_char.diffSplinter Review
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)
Attachment #8511568 - Flags: review?(kent)
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.
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.
I am satisfied with the revised patch( 8511590)
Checked in https://hg.mozilla.org/comm-central/rev/273bcc4d6e9b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 36.0
Attachment #8511568 - Flags: review?(kent)
Attachment #8511590 - Flags: review+
Assignee: nobody → steve
Duplicate of this bug: 1103342
Duplicate of this bug: 1109510
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.
Andreas, 
The patch looks like a good idea to me but you will need to open a new bug for it.
Flags: needinfo?(steve)
Depends on: 1129048
You need to log in before you can comment on or make changes to this bug.