Closed
Bug 918603
Opened 11 years ago
Closed 11 years ago
BaselineJIT.cpp:256:42: warning: comparison is always false due to limited range of data type [-Wtype-limits]
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: dholbert, Assigned: djvj)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
1.62 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
New build warning: { js/src/jit/BaselineJIT.cpp: In function ‘js::jit::MethodStatus CanEnterBaselineJIT(JSContext*, JS::HandleScript, bool)’: js/src/jit/BaselineJIT.cpp:256:42: warning: comparison is always false due to limited range of data type [-Wtype-limits] if (script->nslots > BaselineScript::MAX_JSSCRIPT_SLOTS) ^ } The comparison is ineffective because "nslots" is a uint16_t (assuming I'm looking at the right variable)... > 435 class JSScript : public js::gc::Cell > 436 { [...] > 535 uint16_t nslots; /* vars plus maximum stack depth */ http://mxr.mozilla.org/mozilla-central/source/js/src/jsscript.h#535 ...so, its maximum value is 2^16. In comparison, MAX_JSSCRIPT_SLOTS is 0xfffffu = 2^20 - 1 This constant and the comparison were both added in this cset for bug 905523: http://hg.mozilla.org/mozilla-central/rev/0452b5b504d0
Reporter | ||
Comment 1•11 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #0) > The comparison is ineffective because "nslots" is a uint16_t [...] > ...so, its maximum value is 2^16. (er, make that 2^16 - 1 (not that it matters))
Reporter | ||
Comment 2•11 years ago
|
||
I suspect the constant wanted to be 0xffff (4 f's instead of 5)? But even if that's the case, the comparison is still unnecessary (and would still trigger a build warning), because uint16_t's could reach that value but can never go over it.
Assignee | ||
Comment 3•11 years ago
|
||
Actually I think the assert should just be removed then. I just wanted to make sure that (nslots * sizeof(Value)) would always fit within an int32_t, and if nslots is a uint16_t, then that'll trivially be true.
Assignee | ||
Comment 4•11 years ago
|
||
Fix nonsensical assert.
Attachment #807756 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Updated•11 years ago
|
Assignee: general → kvijayan
Comment 5•11 years ago
|
||
Comment on attachment 807756 [details] [diff] [review] fix-warning.patch Review of attachment 807756 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/BaselineJIT.cpp @@ +252,5 @@ > > if (script->length > BaselineScript::MAX_JSSCRIPT_LENGTH) > return Method_CantCompile; > > + JS_STATIC_ASSERT(sizeof(script->nslots) == sizeof(uint16_t)); nit: Move the comment about the stack depth above this assertion ;)
Attachment #807756 -
Flags: review?(nicolas.b.pierron) → review+
Comment 6•11 years ago
|
||
Comment on attachment 807756 [details] [diff] [review] fix-warning.patch Review of attachment 807756 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/BaselineJIT.cpp @@ +252,5 @@ > > if (script->length > BaselineScript::MAX_JSSCRIPT_LENGTH) > return Method_CantCompile; > > + JS_STATIC_ASSERT(sizeof(script->nslots) == sizeof(uint16_t)); And don't use JS_STATIC_ASSERT! You want C++11 standard static_assert.
Assignee | ||
Comment 7•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a4134bbdae4 https://hg.mozilla.org/integration/mozilla-inbound/rev/512914f8150f
https://hg.mozilla.org/mozilla-central/rev/9a4134bbdae4 https://hg.mozilla.org/mozilla-central/rev/512914f8150f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in
before you can comment on or make changes to this bug.
Description
•