Last Comment Bug 647385 - We should have a ToInteger helper
: We should have a ToInteger helper
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla6
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-01 18:11 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-05-03 11:25 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add it, use it a bunch of obvious places, fix the noted bugs, add tests (10.43 KB, patch)
2011-04-01 18:11 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
cdleary: review+
Details | Diff | Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-01 18:11:05 PDT
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.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-01 18:12:18 PDT
(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 2 Chris Leary [:cdleary] (not checking bugmail) 2011-04-26 16:54:12 PDT
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.
Comment 3 Tom Schuster [:evilpie] 2011-04-26 16:56:31 PDT
Can you add this to jsapi, i think v8monkey needs this.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-26 23:24:12 PDT
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.
Comment 5 Chris Leary [:cdleary] (not checking bugmail) 2011-05-02 16:02:03 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/d433ee7d9f86
Comment 6 :Ms2ger 2011-05-03 05:11:54 PDT
(In reply to comment #2)
> I think we prefer the non-_t variants these days.

AIUI, the opposite is true.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2011-05-03 11:25:43 PDT
(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.)

Note You need to log in before you can comment on or make changes to this bug.