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

Add safe integers #48

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add safe integers #48

wants to merge 1 commit into from

Conversation

toastal
Copy link

@toastal toastal commented Oct 19, 2021

Description of the change

https://tc39.es/ecma262/multipage/numbers-and-dates.html#sec-number.issafeinteger

Add maxSafeInteger, minSafeInteger, and isSafeInteger.


Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

@JordanMartinez
Copy link
Contributor

Same questions I raised in your other PR:

  • Since this is adding FFI, how does this impact other backends?
  • Since this is adding FFI, it's technically a breaking change for other backends.

@JamieBallingall
Copy link

Is this the right library for these constants? An Int is a 32-bit signed integer and this library provides "Functions and bitwise operators for the Int numeric type". Whereas these constants are Number and can't be represented as an Int.

They could be added to Data.Number, although we don't have any indication that they are needed over there.

A sensible place might be in the Int53 library but that already has a Bounded instance and so bottom and top are defined.

Given that this functionality is already available in Int53 I think this PR can be closed without merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants