Closed Bug 845021 Opened 11 years ago Closed 11 years ago

ObjectWrapperChild.cpp:483:36: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

(Whiteboard: [js:t])

Attachments

(1 file)

Build warning, in debug builds:
{
js/ipc/ObjectWrapperChild.cpp:483:36: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
}

Looks like this was introduced by this change:
 https://hg.mozilla.org/mozilla-central/diff/ae005ec67376/js/ipc/ObjectWrapperChild.cpp
for bug 733260, which changed this variable "i" from unsigned (jsuint) to signed (int32_t).
The code in question is:
481     int32_t i = JSVAL_TO_INT(v);
482     NS_ASSERTION(i >= 0, "Index of next jsid negative?");
483     NS_ASSERTION(i <= strIds->Length(), "Index of next jsid too large?");
484 
485     if (size_t(i) == strIds->Length()) {
486         *status = JS_TRUE;
487         return JSObject_to_JSVariant(cx, NULL, statep);
488     }
489 
490     *idp = strIds->ElementAt(i);
491     JS_SetReservedSlot(state, sNextIdIndexSlot, INT_TO_JSVAL(i + 1));
https://mxr.mozilla.org/mozilla-central/source/js/ipc/ObjectWrapperChild.cpp

"i" is int32_t because that's what JSVAL_TO_INT returns.

It looks like all of its usages (except for the final INT_TO_JSVAL) are using it as an unsigned value, so it might make sense to just cast it to unsigned once and stick that in a local var.

Or we could just add another size_t cast for the purpose of the assertion. That's the simplest / least invasive solution, so I'll just do that.
Attached patch fix v1Splinter Review
Per end of prev. comment, this trivial fix just adds a size_t cast to the assertion (matching the size_t cast on the next line of code).
Assignee: general → dholbert
Status: NEW → ASSIGNED
Attachment #718051 - Flags: review?(dmandelin)
Comment on attachment 718051 [details] [diff] [review]
fix v1

And except for the |i >= 0| assertion.  Adding another size_t cast is the right thing to do here, given those eleven lines.  rs=me to do that, stealing because I just looked.  :-)
Attachment #718051 - Flags: review?(dmandelin) → review+
Flags: in-testsuite-
Blocks: 845117
Whiteboard: [js:t]
https://hg.mozilla.org/mozilla-central/rev/e6b97651a167
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: