409324 - js_DoubleToECMA(u)Int32 should return jsdouble, not a useless always-true JSBool

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

...from bug 409302 comment 3.

Comment 1

[Robert Sayre]

Attachment: change the return type

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 294238 [details] [diff] [review]
change the return type

>+js_DoubleToECMAInt32(jsdouble d)
> {
>     jsdouble two32 = 4294967296.0;
>     jsdouble two31 = 2147483648.0;
> 
>     if (!JSDOUBLE_IS_FINITE(d) || d == 0) {
>-        *ip = 0;
>-        return JS_TRUE;
>+        return 0;
>     }

Nit: house style does not over-brace single-line consequents.

>+    return (d >= two31) ? (d - two32) : d;

Warning bait on some compilers? Previous code had (int32) cast to show intentional narrowing.

>+uint32
>+js_DoubleToECMAUint32(jsdouble d)

Same two comments apply to this function.

r/a=me with those fixed, please set flags on final checked-in patch. Thanks,

/be

Comment 3

[Brendan Eich [:brendan]]

I saw Waldo suggesting a few more things on IRC...

/be

Comment 4

[Robert Sayre]

Attachment: fix nits, eliminate some locals in jsparse.c

local var elimination as suggested by waldo

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 294256 [details] [diff] [review]
fix nits, eliminate some locals in jsparse.c

Final nit:

>+    return (int32) ((d >= two31) ? (d - two32) : d);

No need to over-parenthesize the subtraction expression. r=me with that picked.

/be

Comment 6

[Robert Sayre]

Attachment: patch for checkin

Comment 7

[Aiko]

Is js_DoubleToECMAInt32 a special case or is this code pattern (function always returns true) something that should be eliminated in general? For example, js_ValueToBoolean could benefit from a similar makeover, too.