824648 - VMFunctions.cpp:367:5: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

Build warning:
VMFunctions.cpp:367:5: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

For this code:
{
> 364 bool
> 365 CharCodeAt(JSContext *cx, HandleString str, int32_t index, uint32_t *code)
> 366 {
> 367     JS_ASSERT(index < str->length());
[...] 
> 373     *code = chars[index];
}

Build warning is because index is signed, whereas JSString::length() is unsigned (a size_t):
https://mxr.mozilla.org/mozilla-central/source/js/src/vm/String.h#256

We probably want to (a) assert that index is nonnegative, and then (b) cast it to a uint32_t before comparing it to length().

Or we could change the CharCodeAt function-signature -- that might be more correct -- but it looks like that'd be more invasive.

Comment 1

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

oops, meant to link to the chunk of code quoted in comment 0. That MXR link is:
 https://mxr.mozilla.org/mozilla-central/source/js/src/ion/VMFunctions.cpp#364

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> Or we could change the CharCodeAt function-signature -- that might be more
> correct -- but it looks like that'd be more invasive.

Changing the function signature might cause inconsistency with the register allocation of IonMonkey.

Asserting that we have no-negative value should be fine because we are supposed to have bounds check before any call to CharCodeAt.

Comment 3

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

Attachment: fix v1

Comment 4

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47a6822f5e3f

Comment 5

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

https://hg.mozilla.org/mozilla-central/rev/47a6822f5e3f