-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Math Extensions proposal to Stage 1 TC39 0716 #227
Conversation
Thanks. I'll completely review it little later, at this moment some comments. |
@@ -176,12 +176,11 @@ | |||
__webpack_require__(225); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you exclude browser bundles from this PR? It will be updated only at release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's everything under client/, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (previously)
Changes per requested. Couple new |
@@ -0,0 +1,4 @@ | |||
// https://www.npmjs.com/package/ecma-proposal-math-extensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use https://rwaldron.github.io/proposal-math-extensions/
link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@zloirock, I've updated for your second round of comments. It should be in order now. I used Math.PI for the fscale/fround test. |
cc @rwaldron what do you think about it? Mainly interesting your position about |
@clavecoder I wanted at first to have a feedback from proposal's champion, but if it's missing... I'll merge it and publish a new release when I'll have some free time. |
Great, I'll try to move on to adding a PR for the more important #225 on Saturday. |
Implementation is based on the polyfill submitted along with the proposal.
[edit] The related TC39 meeting notes have dropped:
https://github.com/rwaldron/tc39-notes/blob/master/es7/2016-07/jul-26.md#9iie-math-extensions
This is my first PR, so it will need some work. I modeled the changes off of
Number.epsilon
andMath.umulh
. The tests pass. Below are a list of open items. Please inform any other ways I can improve my submissions.Open items:
%Math_max%
,%Math_min%
. Can they, should they be modeled in some way? They are used in the specification ofMath.clamp
.Math.umulh
. Should the identifier be exported? If so, how?Math.max
,Math.min
, andMath.PI
directly. Should I create variables instead, as seems to be the pattern?<CR>
s at the end of some files.