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

Support building against integer-simple #147

Merged
merged 1 commit into from
Nov 7, 2019

Conversation

cocreature
Copy link
Contributor

fixes #135

@@ -31,7 +31,9 @@ import Data.IntSet (IntSet)
import qualified Data.List.NonEmpty as NE
import Data.Map (Map)
import Data.Monoid
#if !MIN_VERSION_primitive(0,7,0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an unrelated change to fix the build against newer versions of primitive. It is still not sufficient to get the test suite working on recent GHCs and recent dependencies (I tried 8.6.5) so to test the instance, I’ve simply uncommented everything but Integer in the “Store on all monomorphic instances” tests.

@mgsloan mgsloan merged commit e568675 into mgsloan:master Nov 7, 2019
@mgsloan
Copy link
Owner

mgsloan commented Nov 7, 2019

LGTM, thanks!!

@mgsloan
Copy link
Owner

mgsloan commented Nov 7, 2019

Note: I have some unpushed changes, fixing the other issues, that I will revisit later this evening

@mgsloan
Copy link
Owner

mgsloan commented Nov 8, 2019

@cocreature I've fixed building of the tests and added explicit unit tests for the serialization of Integer: 5608198

I'd really appreciate it if you ran the tests with integer-simple. For some reason I can't seem to build the tests with the flag on, it seems to trigger a variety of different build tool issues:

¯\(ツ)

Definitely have the flag and extra dep in the stack.yaml https://gist.github.com/mgsloan/d92ed7f28d19ebeb469fdb398a59efc0

@cocreature
Copy link
Contributor Author

@mgsloan Note that you need a different GHC bindist for integer-simple, I got mine via nix. I’ve run the tests (after constraining primititive to 0.6.4.0 since the newly added instances break the TH code in the tests). Everything works except for the tests that you have added after my PR. That is expected, I didn’t attempt to match the serialization format used for integer-gmp and instead opted for one that naturally fits with integer-simple.

In principle it would be possible to match the serialization format of integer-gmp, however if that is a goal I think it would be preferable to change the seriailzation format to something that is less dependent on integer-gmp internals. Currently the serialization format is very specific to how integer-gmp works so given that integer-simple works differently (it doesn’t have the small-size optimization, it has a separate 0 value, the digits are an array of words rather than an array of bytes, …) targeting the integer-gmp format is quite tricky and probably a fair bit slower than what we have now.

@mgsloan
Copy link
Owner

mgsloan commented Nov 10, 2019

@cocreature Oh I see. I thought otherwise because this comment got copied when you wrote the integer-simple code:

    -- May as well put in the extra effort to use the same encoding as
    -- used for the newer integer-gmp.

I've pushed changes documenting this mismatch and removing the test

mgsloan added a commit that referenced this pull request Nov 10, 2019
@mgsloan
Copy link
Owner

mgsloan commented Nov 10, 2019

This change is included in 0.6.1, thanks!

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.

Compatibility with integer-simple
2 participants