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

Make `rem a 0` throw an error (just like `mod a 0` resp. `a % 0` does) #576

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@jvoigtlaender
Contributor

jvoigtlaender commented Apr 28, 2016

This fixes https://github.com/elm-lang/core/issues/563. It is in line with the treatment of %/mod as @evancz did here: elm/compiler#262 (comment).

@rtfeldman

This comment has been minimized.

Show comment
Hide comment
@rtfeldman

rtfeldman May 9, 2016

Member

I think #262 is actually outdated; both NaN and Infinity are most definitely a thing in Elm at present:

http://package.elm-lang.org/packages/elm-lang/core/3.0.0/Basics#isNaN
http://package.elm-lang.org/packages/elm-lang/core/3.0.0/Basics#isInfinite

I believe the thinking on NaN and Infinity is "they are part of the IEEE floating point number spec, and adding checks to diverge from that spec has performance costs for operations that get used all over the place, so let's not make that the default."

Pretty sure it would make more sense to remove the check from mod, if anything. 😉

Member

rtfeldman commented May 9, 2016

I think #262 is actually outdated; both NaN and Infinity are most definitely a thing in Elm at present:

http://package.elm-lang.org/packages/elm-lang/core/3.0.0/Basics#isNaN
http://package.elm-lang.org/packages/elm-lang/core/3.0.0/Basics#isInfinite

I believe the thinking on NaN and Infinity is "they are part of the IEEE floating point number spec, and adding checks to diverge from that spec has performance costs for operations that get used all over the place, so let's not make that the default."

Pretty sure it would make more sense to remove the check from mod, if anything. 😉

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender May 9, 2016

Contributor

@rtfeldman, all the arguments you give apply to Float, and only to Float. Including the arguments about the existence of isNaN and isInfinite. Both of these have type Float -> Bool. But #262 was about Int, not Float. And both mod and rem are operations on Int. So I don't really see how your reasoning applies.

But in any case, I agree that most importantly mod and rem should behave consistently. Whether that is by both allowing division by zero or both not allowing it, well, ...

Contributor

jvoigtlaender commented May 9, 2016

@rtfeldman, all the arguments you give apply to Float, and only to Float. Including the arguments about the existence of isNaN and isInfinite. Both of these have type Float -> Bool. But #262 was about Int, not Float. And both mod and rem are operations on Int. So I don't really see how your reasoning applies.

But in any case, I agree that most importantly mod and rem should behave consistently. Whether that is by both allowing division by zero or both not allowing it, well, ...

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender May 9, 2016

Contributor

But if rem a 0 stays allowed and a % 0 becomes allowed, and then naturally a // 0 as well, then isNaN and isInfinite should be adapted to have type number -> Bool rather than Float -> Bool, because they should become applicable to Int as well.

Contributor

jvoigtlaender commented May 9, 2016

But if rem a 0 stays allowed and a % 0 becomes allowed, and then naturally a // 0 as well, then isNaN and isInfinite should be adapted to have type number -> Bool rather than Float -> Bool, because they should become applicable to Int as well.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender May 10, 2016

Contributor

Closing this for the more comprehensive https://github.com/elm-lang/core/issues/590.

@rtfeldman, what do you think about that new issue?

Contributor

jvoigtlaender commented May 10, 2016

Closing this for the more comprehensive https://github.com/elm-lang/core/issues/590.

@rtfeldman, what do you think about that new issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment