Closed
Bug 299560
Opened 19 years ago
Closed 19 years ago
nsCSSParser.cpp:935 Comparison between signed and unsigned integer expressions
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bastiaan, Assigned: bastiaan)
References
(Blocks 1 open bug, )
Details
Attachments
(1 file, 1 obsolete file)
1.80 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
benjamin
:
approval1.8b4+
|
Details | Diff | Splinter Review |
> sub_end = aBuffer.FindChar(PRUnichar(','), sub); sub_end is unsigned; however, FindChar() may return -1. > if (sub_end == -1) is therefore always false.
Assignee | ||
Comment 1•19 years ago
|
||
Comment 2•19 years ago
|
||
Why would it be false? In both cases the -1 is coerced to unsigned before the comparison is done...
Updated•19 years ago
|
Attachment #188125 -
Flags: superreview?(dbaron)
Attachment #188125 -
Flags: review?(dbaron)
Assignee | ||
Updated•19 years ago
|
Attachment #188125 -
Flags: superreview?(bzbarsky)
Attachment #188125 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•19 years ago
|
Attachment #188125 -
Flags: superreview?(bzbarsky)
Attachment #188125 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 3•19 years ago
|
||
(In reply to comment #2) > Why would it be false? In both cases the -1 is coerced to unsigned before the > comparison is done... Perhaps I should have said, it would "technically speaking" be false. But at least fixing this will shut the compiler up.. :)
Comment on attachment 188125 [details] [diff] [review] fix If you want to fix the warning, use nsSubstring::index_type(kNotFound) instead of -1. FindChar really *should* be returning index_type. And then run Ian's testcases (see the bug for which this code was checked in).
Attachment #188125 -
Flags: superreview?(dbaron)
Attachment #188125 -
Flags: superreview-
Attachment #188125 -
Flags: review?(dbaron)
Attachment #188125 -
Flags: review-
(Really, all the |PRUint32|s should be |nsSubstring::index_type|s, so PRUint32(kNotFound) is fine too.)
Assignee | ||
Comment 6•19 years ago
|
||
(In reply to comment #5) > (Really, all the |PRUint32|s should be |nsSubstring::index_type|s, so > PRUint32(kNotFound) is fine too.) So you're suggesting an explicit conversion of -1 to zero, right? Shouldn't we account for the case when FindChar actually returns zero, though?
Comment 7•19 years ago
|
||
Why to zero? It'd convert to PR_UNIT32_MAX, basically...
Assignee | ||
Comment 8•19 years ago
|
||
You're right, of course.. I wrongly assumed -1 would be cast to 0.
Attachment #188125 -
Attachment is obsolete: true
Assignee | ||
Comment 9•19 years ago
|
||
Comment on attachment 188133 [details] [diff] [review] another fix (In reply to comment #4) > And then run Ian's > testcases (see the bug for which this code was checked in). Firefox with this patch applied passes all of Hixie's tests.
Attachment #188133 -
Flags: superreview?(dbaron)
Attachment #188133 -
Flags: review?(dbaron)
Attachment #188133 -
Flags: superreview?(dbaron)
Attachment #188133 -
Flags: superreview+
Attachment #188133 -
Flags: review?(dbaron)
Attachment #188133 -
Flags: review+
Assignee | ||
Updated•19 years ago
|
Severity: normal → trivial
Assignee | ||
Updated•19 years ago
|
Blocks: buildwarning
Assignee | ||
Updated•19 years ago
|
Attachment #188133 -
Flags: approval1.8b4?
Updated•19 years ago
|
Attachment #188133 -
Flags: approval1.8b4? → approval1.8b4+
Assignee | ||
Comment 10•19 years ago
|
||
Checked in by timeless (2005-07-13 11:16).
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•