mgsloan / store

Fast binary serialization in Haskell
MIT License
109 stars 35 forks source link

Support building against integer-simple #147

Closed cocreature closed 4 years ago

cocreature commented 4 years ago

fixes #135

mgsloan commented 4 years ago

LGTM, thanks!!

mgsloan commented 4 years ago

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

mgsloan commented 4 years ago

@cocreature I've fixed building of the tests and added explicit unit tests for the serialization of Integer: https://github.com/fpco/store/commit/560819846c6920f8e8c47095daa51ce25349bb2b

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

This change is included in 0.6.1, thanks!