Closed
Bug 1021963
Opened 10 years ago
Closed 10 years ago
Self-host isNaN and isFinite, relying on Number_isNaN and Number_isFinite for implementation
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: Waldo, Assigned: jj, Mentored)
Details
(Whiteboard: [good first bug][lang=c++])
Attachments
(1 file, 1 obsolete file)
4.93 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
We have C++ implementations of isNaN and isFinite now, but there's no real reason for that. We should self-host them, and we should change all current self-hosted that uses them to instead use Number.isNaN and Number.isFinite (which means that the self-hosted code should use Number_isNaN and Number_isFinite).
Comment 1•10 years ago
|
||
Hi, I'm new to the mozilla developer community and I am interested to take this bug on as my first. It would be really great if you could mentor me.
Reporter | ||
Comment 2•10 years ago
|
||
Awesome! First step is to do a developer/debug build of SpiderMonkey if you haven't already, see here for directions: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation Once that's done, you're going to need to remove the global-method isNaN and isFinite implementations from js/src/jsnum.cpp. And to replace them with self-hosted versions, you'll want to add JS definitions of them to js/src/builtin/Number.js. Those definitions should be basically this simple: function Global_isNaN(number) { return Number_isNaN(ToNumber(number)); } function Global_isFinite(number) { return Number_isFinite(ToNumber(number)); } To hook these up for actual use, just change the JSFunctionSpec lines that hook up the global isNaN/isFinite to look like the lines elsewhere in the file that hook up self-hosted code. The lines for Number.isFinite/isNaN will be helpful here. If you have any other questions, don't hesitate to ask.
Updated•10 years ago
|
Mentor: jwalden+bmo
Whiteboard: [good first bug][lang=c++][mentor=jwalden] → [good first bug][lang=c++]
Comment 5•10 years ago
|
||
Hi, Debkanya - it looks like this bug is spoken for at the moment. Please contact me at mhoye@mozilla.com and we'll try to find you a different bug that's a good fit.
Comment 6•10 years ago
|
||
Well, given that it's been three weeks since we've heard from Abhinandan, I'd say you should just go ahead and work on this. Abhinandan, if you're actively working on this, please shout. Otherwise, rest assured that this won't be the last bug in need of an owner. :)
Comment 7•10 years ago
|
||
Hi Debkanya, I have started working on this bug a week ago. Sorry, I'm working very slowly. Could you find another one? I will submit the patch for this soon.
Assignee | ||
Comment 9•10 years ago
|
||
I gave a try to this bug please have a look at this implementation
Attachment #8477862 -
Flags: review?(jwalden+bmo)
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8477862 [details] [diff] [review] Self-host isNaN and isFinite Review of attachment 8477862 [details] [diff] [review]: ----------------------------------------------------------------- Great! I'll drop this into my tree and land it shortly. Thanks for the patch! You have any ideas about what you want to do next?
Attachment #8477862 -
Flags: review?(jwalden+bmo) → review+
Reporter | ||
Comment 11•10 years ago
|
||
Actually, wait. Your patch builds, but it doesn't actually work. We have self-hosting code that expects isNaN and isFinite to be not-selfhosted, under the std_isFinite and std_isNaN monikers. All of those have to be converted to use Number_isFinite and Number_isNaN, for this to work. Also, the now-unused C++ functions for these should be removed. This work's simple enough that I just did it locally. But really, you need to test your patches. :-) At least starting up the JS shell should work before you attach a patch -- and really, it should pass all jit-tests and jstests: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Running_Automated_JavaScript_Tests Anyway, I've made the changes this time. But next time, please take these extra steps to ensure the patches you upload are ready to go.
Attachment #8477862 -
Attachment is obsolete: true
Attachment #8479422 -
Flags: review+
Reporter | ||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/15109655e5f0
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/15109655e5f0
Assignee: nobody → jinank94
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•