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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bastiaan Jacques]

> sub_end = aBuffer.FindChar(PRUnichar(','), sub);
sub_end is unsigned; however, FindChar() may return -1.
> if (sub_end == -1)
is therefore always false.

Comment 1

[Bastiaan Jacques]

Attachment: fix

Comment 2

[Boris Zbarsky [:bzbarsky]]

Why would it be false?  In both cases the -1 is coerced to unsigned before the
comparison is done...

Comment 3

[Bastiaan Jacques]

(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 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(Really, all the |PRUint32|s should be |nsSubstring::index_type|s, so
PRUint32(kNotFound) is fine too.)

Comment 6

[Bastiaan Jacques]

(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

[Boris Zbarsky [:bzbarsky]]

Why to zero?  It'd convert to PR_UNIT32_MAX, basically...

Comment 8

[Bastiaan Jacques]

Attachment: another fix

You're right, of course.. I wrongly assumed -1 would be cast to 0.

Comment 9

[Bastiaan Jacques]

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.

Comment 10

[Bastiaan Jacques]

Checked in by timeless (2005-07-13 11:16).