If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla22

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla22
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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).
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 718051 [details] [diff] [review]
fix v1

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+
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6b97651a167
(Assignee)

Updated

5 years ago
Flags: in-testsuite-
(Assignee)

Updated

5 years ago
Blocks: 845117
Whiteboard: [js:t]
https://hg.mozilla.org/mozilla-central/rev/e6b97651a167
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.