Closed Bug 647385 Opened 9 years ago Closed 9 years ago

We should have a ToInteger helper


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)


(Whiteboard: fixed-in-tracemonkey)


(1 file)

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)
      throw "should have thrown";
    catch (e)
      assertEq(e instanceof RangeError, true);
  test(Math.pow(2, 32) + 10);

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.

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
Closed: 9 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.