We should have a ToInteger helper

RESOLVED FIXED in mozilla6

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

Created attachment 523734 [details] [diff] [review]
Add it, use it a bunch of obvious places, fix the noted bugs, add tests

We have 'em for most of the To* spec methods, but not for this one.  Also, while doing this I also discovered a couple bugs easily fixed mostly in-passing:

  function test(r)
  {
    try
    {
      5..toString(r);
      throw "should have thrown";
    }
    catch (e)
    {
      assertEq(e instanceof RangeError, true);
    }
  }
  test(Math.pow(2, 32) + 10);
  test(55);

We didn't do ToInteger when handling the radix, as the first bug.  And we didn't throw a RangeError when handling a bad radix, as the second.
Attachment #523734 - Flags: review?(cdleary)
(There are a lot more places in the spec that use ToInteger than just the ones fixed here, but many are intertwined with other range checks, or NaN checks, or whatever, so I restricted changes here to the obvious and easy cases.)
Comment on attachment 523734 [details] [diff] [review]
Add it, use it a bunch of obvious places, fix the noted bugs, add tests

+    if (!ToInteger(cx, *argv, &d))
         return JS_FALSE;

While we're cleaning stuff up can we make this function uniformly return the C++ bools?

+    else if (!ValueToNumber(cx, v, dp))
+        return false;

I think this can be ValueToNumberSlow, since isNumber is (isInt32 || isDouble).

+        base = int32_t(d2);

I think we prefer the non-_t variants these days.

Great stuff -- I love the smell of cleanup in the morning! Though, that may just be the smell of the cleaning chemicals.
Attachment #523734 - Flags: review?(cdleary) → review+
Can you add this to jsapi, i think v8monkey needs this.
http://hg.mozilla.org/tracemonkey/rev/d433ee7d9f86

I'll poke people about v8 needing an API for this tomorrow.  Also, I'd rather discuss an API in a separate bug, as this one was solely targeted at cleanup and simplification.
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla6
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/d433ee7d9f86
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(In reply to comment #2)
> I think we prefer the non-_t variants these days.

AIUI, the opposite is true.
(In reply to comment #6)
> (In reply to comment #2)
> > I think we prefer the non-_t variants these days.
> 
> AIUI, the opposite is true.

You're mostly wrong, unfortunately.  (As for why I had it this way, it's because the existing variable was that way and I wanted to minimize size.)
You need to log in before you can comment on or make changes to this bug.