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

feat(stdlib): isNaN and isFinite utility functions for Number #729

Merged
merged 8 commits into from
Jun 15, 2021

Conversation

cician
Copy link
Contributor

@cician cician commented Jun 11, 2021

This pull request adds two functions to the number module:

isFinite: (Number) -> Bool
isNaN: (Number) -> Bool

So we want explicit isInfinity and isNegativeInfinity as well?

Internally they depend on isBoxedNumber from runtime/numbers so if you don't mind I'd export it.

Technically there's duplicate code in numberUtils, but it's only for WasmF64 and just two lines of actual code. So I don't know if we want to bother reusing it.

Tests could benefit from waiting for PR #720 I guess.

@peblair peblair requested a review from a team June 13, 2021 11:45
Copy link
Member

@peblair peblair left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great! A couple of notes (aside from the suggested comments):

  • We indent the code in the standard library with two spaces, so if you could update these functions to match that style, it would be very helpful.
  • With respect to other useful functions, I think an isInfinite function (returning true for positive and negative infinity) would round all of this out nicely. This is what other languages (e.g. Python, Racket) appear to do. I'm not strictly opposed to isPositiveInfinity/isNegativeInfinity as well, but I am not sure it's necessary when users can do isInfinite(x) && x > 0. Wondering what @ospencer's thoughts are.
  • Finally, when making the above change(s), can you rebase this onto the latest main?

Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

I love how readable this is, even to me!

Copy link
Member

@ospencer ospencer left a comment

Choose a reason for hiding this comment

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

Looks awesome! I agree with @peblair that isInfinite is a fine choice here 👍

Left you some tiny tweaks to your code comments, but otherwise this is perfect!

cician and others added 2 commits June 14, 2021 18:04
Co-authored-by: Oscar Spencer <oscar.spen@gmail.com>
Co-authored-by: Oscar Spencer <oscar.spen@gmail.com>
Copy link
Member

@ospencer ospencer left a comment

Choose a reason for hiding this comment

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

Thanks so much @cician!

@ospencer ospencer merged commit b907da7 into grain-lang:main Jun 15, 2021
@github-actions github-actions bot mentioned this pull request May 31, 2022
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.

4 participants