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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

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.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 400657 [details] [diff] [review]
Patch

Thanks.

Comment 3

[Igor Bukanov]

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

Comment 4

[Igor Bukanov]

(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));

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Axe all warnings, don't overflow signed values anywhere

Comment 6

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/467758d5e9f1
http://hg.mozilla.org/tracemonkey/rev/af2c906d75a8

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/467758d5e9f1
http://hg.mozilla.org/mozilla-central/rev/af2c906d75a8

Comment 8

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/87c648233bfe