Switch to Euclidean division for Int, resolves #161 #168
Conversation
Also provide `quot` and `rem`, like Haskell does, for users who do want truncating division - the one which matches what JS does. I've temporarily exported `intDiv` and `intMod` so that I can use those in the tests and the compiler won't 'inline' different definitions of them; we'll want to modify the compiler to change this before merging.
src/Data/EuclideanRing.js
Outdated
| exports.intDiv = function (x) { | ||
| return function (y) { | ||
| return Math.sign(y) * Math.floor(x / Math.abs(y)); |
There was a problem hiding this comment.
Math.sign is ES2015 unfortunately
There was a problem hiding this comment.
I guess the simplest alternative is something like:
return y > 0 ? Math.floor(x / y) : -Math.floor(x / -y);There was a problem hiding this comment.
Ah, thanks, I wasn't aware of that.
| -- | mod 2 (-3) == 2 | ||
| -- | rem 2 (-3) == 2 | ||
| -- | ``` | ||
| foreign import rem :: Int -> Int -> Int |
There was a problem hiding this comment.
Perhaps we should provide quot and rem in purescript-integers, since they are Int specialised, rather than constrained to EuclideanRing?
There was a problem hiding this comment.
Yeah, that wouldn't be a bad option. I think it makes sense to put them here, because they do form a valid EuclideanRing instance (even if we're no longer using it as the default instance for Int), and also because these functions were available from Prelude before, but I don't have a strong preference.
src/Data/EuclideanRing.purs
Outdated
| -- | negative infinity if the divisor is positive, and towards positive infinity | ||
| -- | if the divisor is negative. Note that all three definitions are identical if | ||
| -- | we restrict our attention to nonnegative dividends and divisors. | ||
|
|
|
This looks good to me, aside from those few comments above. I've opened a PR to remove |
|
Ah no, we do, but it's done differently than the mod case. I'll update that PR accordingly. |
|
Thanks again for working on this! |
|
Happy to :) just one thing though - I just realised that the code here still exports |
|
Yeah, thanks for the reminder - I went ahead and did that already. I did move |
Also provide
quotandrem, like Haskell does, for users who do wanttruncating division - the one which matches what JS does.
I've temporarily exported
intDivandintModso that I can use thosein the tests and the compiler won't 'inline' different definitions of
them; we'll want to modify the compiler to change this before merging.
I've also done a more detailed writeup in a separate repo but I thought that might be a bit much for these docs.