Closed Bug 848159 Opened 7 years ago Closed 7 years ago

js/src/jsopcode.cpp:1659:102: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

New build warning:
{
js/src/jsopcode.cpp: In function ‘bool DecompileArgumentFromStack(JSContext*, int, char**)’:

js/src/jsopcode.cpp:1659:102: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
}

...from this line of code:
> 1659     if (JSOp(*current) != JSOP_CALL || formalIndex >= GET_ARGC(current))
https://mxr.mozilla.org/mozilla-central/source/js/src/jsopcode.cpp#1659

...which was introduced by bug 846080's cset https://hg.mozilla.org/mozilla-central/rev/1ab536b3472e
(I'm hitting this on GCC 4.7 on linux, but I expect most or all compilers will warn about this.)
This function starts with
   JS_ASSERT(formalIndex >= 0);
so presumably we're justified in assuming formalIndex is nonnegative in the comparison quoted in comment 0. (and hence we'd be fine casting it to be unsigned)
Here's a patch that just adds a static_cast<unsigned> for the purpose of the comparison. The patch has extra amounts of contextual lines, to show the JS_ASSERT that I quoted in my previous comment (which is what makes this cast justified).

NOTE:
I picked "unsigned" as the type for the static_cast (instead of e.g. size_t or uintNN_t) because that's the type that GET_ARGC returns. GET_ARG is defined as:
> 197 #define GET_ARGC(pc)            GET_UINT16(pc)
https://mxr.mozilla.org/mozilla-central/source/js/src/jsopcode.h#197

...which is defined as:
> 128 #define GET_UINT16(pc)          ((unsigned)(((pc)[1] << 8) | (pc)[2]))
https://mxr.mozilla.org/mozilla-central/source/js/src/jsopcode.h#128
Assignee: general → dholbert
Status: NEW → ASSIGNED
Attachment #725448 - Flags: review?(jdemooij)
Comment on attachment 725448 [details] [diff] [review]
fix v1: static_cast<unsigned>

Review of attachment 725448 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsopcode.cpp
@@ +1650,5 @@
>          return true;
>  
>      /* Don't handle getters, setters or calls from fun.call/fun.apply. */
> +    if (JSOp(*current) != JSOP_CALL ||
> +        static_cast<unsigned>(formalIndex) >= GET_ARGC(current))

Style nit: multi-line condition, so body gets { }, with opening brace on its own line:

if (aa ||
    bb)
{
    ...
}

However, looks like this also fits on one line (limit is 99 characters for SM code). Either of these is fine with me.
Attachment #725448 - Flags: review?(jdemooij) → review+
Ah, I'll just keep it on one line, then. Thanks! I'll land that shortly.

(I'd initially thought about adding curly-braces, but I didn't because the contextual code is just a bunch of other one-line non-braced conditional returns, and the one lone braced-return looked out-of-place. :))
https://hg.mozilla.org/mozilla-central/rev/267ade662734
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.