Skip to content
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

Fix the polyfill of Math.fround. #12535

Merged
merged 2 commits into from
Apr 1, 2022
Merged

Fix the polyfill of Math.fround. #12535

merged 2 commits into from
Apr 1, 2022

Conversation

sjrd
Copy link
Contributor

@sjrd sjrd commented Jan 31, 2022

Summary

Fix the polyfill of Math.fround that does not rely on Float32Array.

Motivation

As shown here:
https://gist.github.com/shicks/7a97ec6b3f10212e60a89a7f6d2d097d?permalink_comment_id=4048135#gistcomment-4048135
the old polyfill was incorrect, because it was breaking ties upwards when rounding. The spec of fround requires to break ties to even instead.

The new polyfill correctly breaks ties to even, even at the critical points of transitioning from subnormal to normal forms, and from finite to infinity.

To perform ties-to-even, the code relies on the natural rounding that will happen when getting into the subnormal forms of (64-bit) numbers.

In addition, the new polyfill does not assume that Math.log will always return the best approximation. The spec of Math.log explicitly says that it may not return the best approximation.

Supporting details

Metadata

  • Adds a new document
  • Rewrites (or significantly expands) a document
  • Fixes a typo, bug, or other error

As shown here:
https://gist.github.com/shicks/7a97ec6b3f10212e60a89a7f6d2d097d?permalink_comment_id=4048135#gistcomment-4048135
the old polyfill was incorrect, because it was breaking ties *upwards*
when rounding. The spec of `fround` requires to break ties to *even*
instead.

The new polyfill correctly breaks ties to even, even at the critical
points of transitioning from subnormal to normal forms, and from
finite to infinity.

To perform ties-to-even, the code relies on the natural rounding
that will happen when getting into the subnormal forms of
(64-bit) `number`s.

In addition, the new polyfill does not assume that `Math.log` will
always return the best approximation. The spec of `Math.log`
explicitly says that it may not return the best approximation.
@sjrd sjrd requested a review from a team as a code owner January 31, 2022 14:27
@sjrd sjrd requested review from wbamberg and removed request for a team January 31, 2022 14:27
@github-actions github-actions bot added the Content:JS JavaScript docs label Jan 31, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2022

Preview URLs

Flaws

None! 🎉

External URLs

URL: /en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/fround
Title: Math.fround()
on GitHub

(this comment was updated 2022-04-01 15:11:17.339589)

@tshemsedinov
Copy link
Contributor

@sjrd Note this: openwebdocs/project#27

@sjrd
Copy link
Contributor Author

sjrd commented Feb 9, 2022

Thank you for the link. I read the issue and followed some pointers from there. Following the rabbit hole, I ended up discovering Veltkamp's splitting algorithm, which improves the normal form case significantly.

I'm not sure what to make of the linked issue wrt. this PR, though. Does that mean that no new PR updating MDN polyfills is going to be accepted, even until a solution is decided? If there is a chance for this to be merged, I'll update the PR with our newest implementation.

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to be slow @sjrd . I think that since we have a link to the core-js polyfill in this page, we should delete this polyfill. Although we don't unfortunately have a very clear policy here, this is more or less in line with our recent practice.

A polyfill is available in core-js, and already referenced in the
See also section.
@sjrd
Copy link
Contributor Author

sjrd commented Apr 1, 2022

All right. I added a commit that removes the Polyfill section entirely.

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @sjrd !

@wbamberg wbamberg merged commit fa48827 into mdn:main Apr 1, 2022
@sjrd sjrd deleted the patch-1 branch April 1, 2022 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:JS JavaScript docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants