Closed Bug 824648 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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
(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.
Attached patch fix v1Splinter Review
Assignee: general → dholbert
Status: NEW → ASSIGNED
Attachment #695764 - Flags: review?(nicolas.b.pierron)
Attachment #695764 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/47a6822f5e3f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: