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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Daniel Holbert [:dholbert]]

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

Comment 1

[Daniel Holbert [:dholbert]]

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.

Comment 2

[Daniel Holbert [:dholbert]]

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

Comment 3

[Jeff Walden [:Waldo]]

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

Comment 4

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6b97651a167

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e6b97651a167