-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fix build with -integer-gmp #7
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some notes about by pull request.
@@ -616,7 +618,8 @@ hash32BigNatByteArrayBytes h numLimbs ba = | |||
instance Hashable Natural where | |||
{-# INLINE hash #-} | |||
hash h nat = case nat of | |||
# if defined (MIN_VERSION_integer_gmp) && MIN_VERSION_integer_gmp(1,0,0) | |||
# if defined (MIN_VERSION_integer_gmp) | |||
# if MIN_VERSION_integer_gmp(1,0,0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to separate these 2 checks. Combining with &&
results in an error:
if defined X && X(1,0,0)
Consider the case where X is indeed not defined:
if defined && (1,0,0)
No you will get some error complaining about terniary operators.
@@ -626,9 +629,13 @@ instance Hashable Natural where | |||
# endif | |||
-- Else using a BigNat (which instance calls required mixConstructor): | |||
(NatJ# bn) -> hash h bn | |||
# else | |||
-- Natural represented with non-negative Integer: | |||
n -> hash h $ toInteger n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The recommended interface to Natural
is Numeric.Natural which does not expose a constructor. Without the integer-gmp
flag I got an error because the Natural
constructor was not in scope. So I used the Integral
interface.
@@ -72,7 +72,9 @@ import GHC.Integer.GMP.Internals (BigNat(BN#)) | |||
#if MIN_VERSION_base(4,8,0) | |||
import Data.Void (Void, absurd) | |||
import GHC.Natural (Natural(..)) | |||
# ifdef MIN_VERSION_integer_gmp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import was unused with the integer-gmp
flag disabled.
Thanks a lot! Obviously I never tried running with that flag toggled :/ If you have time to look at the tests too, that would be great:
They should compile, but also we'd like that values hash the same when compiled with and without integer-gmp (especially for the GHCJS use-case); if I don't have a golden test for Natural yet it should be added. |
I should also see if I can get travis to run the tests and compilation with and without |
Good point about the tests. There are indeed similar problems as with the normal library. I'll have a commit ready shortly. |
I fixed the test-suite. I ran the tests with and without the Running with GHCJS-7.10.3 results in a failure. I will create a separate ticket for that. |
This change was necessary in order to build hashabler with GHCJS.