Closed Bug 516623 Opened 10 years ago Closed 10 years ago

jsnum.cpp:87: warning: integer overflow in expression

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: mrbkap, Assigned: Waldo)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files)

This is due to:

JS_STATIC_ASSERT(int32_t(INT32_MAX + 1) == INT32_MIN);

and related assertions. While it's always nice to have sanity checks, I don't think these assertions are worth the warning.
Attached patch PatchSplinter Review
Kills asserts when stdint.h is used (good compilers), silences warnings when MSVC (bad compiler) is used (tested on mini on my desk), should cover everybody, I think -- and even if not, out of sight, out of mind.
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #400657 - Flags: review?(mrbkap)
Comment on attachment 400657 [details] [diff] [review]
Patch

Thanks.
Attachment #400657 - Flags: review?(mrbkap) → review+
(In reply to comment #0)
> This is due to:
> 
> JS_STATIC_ASSERT(int32_t(INT32_MAX + 1) == INT32_MIN);
> 
> and related assertions. While it's always nice to have sanity checks, I don't
> think these assertions are worth the warning.

The assert should use unsigned arithmetic like in:

JS_STATIC_ASSERT(uint32_t(INT32_MAX + 1) == uint32_t(INT32_MIN));

C/C++ standards does not specify the behavior of int arithmetic regarding the overflow while they explicitly state the wrap around for unsigned types.
(In reply to comment #3)
> (In reply to comment #0)
> The assert should use unsigned arithmetic like in:
> 
> JS_STATIC_ASSERT(uint32_t(INT32_MAX + 1) == uint32_t(INT32_MIN));

I meant:

JS_STATIC_ASSERT(uint32_t(INT32_MAX) + uint32_t(1) == uint32_t(INT32_MIN));
Attachment #400837 - Flags: review?(mrbkap) → review+
http://hg.mozilla.org/tracemonkey/rev/467758d5e9f1
http://hg.mozilla.org/tracemonkey/rev/af2c906d75a8
OS: Linux → All
Hardware: x86 → All
Whiteboard: fixed-in-tracemonkey
Flags: wanted1.9.2+
You need to log in before you can comment on or make changes to this bug.