Closed Bug 299560 Opened 14 years ago Closed 14 years ago

nsCSSParser.cpp:935 Comparison between signed and unsigned integer expressions

Categories

(Core :: CSS Parsing and Computation, defect, trivial)

x86
Linux
defect
Not set
trivial

Tracking

()

RESOLVED FIXED

People

(Reporter: bastiaan, Assigned: bastiaan)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 1 obsolete file)

> sub_end = aBuffer.FindChar(PRUnichar(','), sub);
sub_end is unsigned; however, FindChar() may return -1.
> if (sub_end == -1)
is therefore always false.
Attached patch fix (obsolete) — Splinter Review
Why would it be false?  In both cases the -1 is coerced to unsigned before the
comparison is done...
Attachment #188125 - Flags: superreview?(dbaron)
Attachment #188125 - Flags: review?(dbaron)
Attachment #188125 - Flags: superreview?(bzbarsky)
Attachment #188125 - Flags: review?(bzbarsky)
Attachment #188125 - Flags: superreview?(bzbarsky)
Attachment #188125 - Flags: review?(bzbarsky)
(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.)
(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?
Why to zero?  It'd convert to PR_UNIT32_MAX, basically...
Attached patch another fixSplinter Review
You're right, of course.. I wrongly assumed -1 would be cast to 0.
Attachment #188125 - Attachment is obsolete: true
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+
Severity: normal → trivial
Blocks: buildwarning
Attachment #188133 - Flags: approval1.8b4?
Attachment #188133 - Flags: approval1.8b4? → approval1.8b4+
Checked in by timeless (2005-07-13 11:16).
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.