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 BigInt bindings #5350

Closed
wants to merge 6 commits into from
Closed

Conversation

tom-sherman
Copy link

Relates to #4677.

This is my first ever ocaml code, would appreciate if someone could help getting the PR ready to be merged - or just outline what I need to do. Thanks!


external ofString : string -> t = "BigInt" [@@bs.val]
external ofInt : int -> t = "BigInt" [@@bs.val]
external ofFloat : float -> t = "BigInt" [@@bs.val]
Copy link
Author

Choose a reason for hiding this comment

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

This is dangerous enough that I think we may not want to include it.

The ReScript expression ofFloat(99999999999999999999999999999.) for example will produce the value 99999999999999991433150857216n as the float literal will be truncated by the JS environment.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

What OCaml version is being used to compile ReScript nowadays? Newer OCaml versions have an 'alert' feature that trigger a warning if you use some piece of code. E.g. I'm doing that in https://github.com/yawaramin/ocaml-decimal/blob/08c57183c8673b5058bd6010570e69f0201c03c7/lib/decimal.mli#L321

If that's not available, perhaps we can mark this function as deprecated with the @deprecated tag.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@yawaramin yawaramin left a comment

Choose a reason for hiding this comment

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

Looks good overall, we will also need the comparison and equality operators.


external ofString : string -> t = "BigInt" [@@bs.val]
external ofInt : int -> t = "BigInt" [@@bs.val]
external ofFloat : float -> t = "BigInt" [@@bs.val]
Copy link
Contributor

Choose a reason for hiding this comment

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

What OCaml version is being used to compile ReScript nowadays? Newer OCaml versions have an 'alert' feature that trigger a warning if you use some piece of code. E.g. I'm doing that in https://github.com/yawaramin/ocaml-decimal/blob/08c57183c8673b5058bd6010570e69f0201c03c7/lib/decimal.mli#L321

If that's not available, perhaps we can mark this function as deprecated with the @deprecated tag.

jscomp/others/js_bigint.ml Outdated Show resolved Hide resolved
@cannorin
Copy link
Contributor

cannorin commented Jun 8, 2022

ts2ocaml would greatly benefit from having bigint defined & supported in rescript.

I'm willing to help this if you need a hand.

@cristianoc
Copy link
Collaborator

How about reviving this?
First by rebasing on master, then revisiting any open questions left.

@DZakh
Copy link
Contributor

DZakh commented Jul 9, 2022

I've created a PR in which added the BigInt module #5531. I think it should be good enough as the first iteration.

@bobzhang
Copy link
Member

I am thinking we may provide native bigint in the long term

@cristianoc
Copy link
Collaborator

ts2ocaml would greatly benefit from having bigint defined & supported in rescript.

I'm willing to help this if you need a hand.

What's required specifically for ts2ocaml? Frankly speaking, that project has great potential and I'm simply interested in removing obstacles in the way to see what comes out of it.

@cannorin
Copy link
Contributor

#5539 solved the core problem, as ts2ocaml now don't have to define bigint on our own and just use BigInt.t.

It would certainly help if we can have a proper bigint binding, but it doesn't need to be available as soon as possible 🙂

@tom-sherman
Copy link
Author

Going to close this PR now that we have #5539. I wanted to add these so that we can have a shared bigint type across third party code, the bindings are not so important to me.

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.

6 participants