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

Is custom version of Data.Binary still needed? #82

Open
inariksit opened this issue Nov 9, 2020 · 2 comments
Open

Is custom version of Data.Binary still needed? #82

inariksit opened this issue Nov 9, 2020 · 2 comments
Labels
question Further information is requested

Comments

@inariksit
Copy link
Member

inariksit commented Nov 9, 2020

This is not urgent by any means, but I'm bringing this up so that it's not buried in old commit messages.

The GF codebase includes a modified version of Haskell's Data.Binary from 2008. According to Grégoire's commit from 2010, this modification was necessary, because the binary representation of doubles in Data.Binary was (is?) not compliant with IEEE 754.

In the commit history, you can see a comment from 2013:

The standard binary package has improved efficiency and error handling [1], so
in the long run we should consider switching to it.

Is this still the case? The code from 2008 contains some unsafe IO operations (inlinePerformIO is deprecated and has a newer, more accurate name accursedUnutterablePerformIO). Later versions of Data.Binary don't contain accursedUnutterable…, at least not in those functions I checked.

Maybe more interesting, the implementation of the Get monad has changed, see current version of Binary vs. old version used in GF.

Ways to proceed

From least work to most work:

  1. No real changes: Keep the modified Data.Binary from 2008 with all its curses, accursedUnutterablePerformIO is actually necessary in our code (for performance reasons?).
  2. Keep the modified Data.Binary from 2008, but replace accursed unutterable functions with something safer, like unsafePerformIO.
  3. If Data.Binary has fixed its representation of doubles and any other issues for which the custom module was used: Switch to the latest version of Data.Binary library. (+Write some proper property-based tests to make sure that the change doesn't break GF.)
@inariksit inariksit added the question Further information is requested label Nov 9, 2020
@johnjcamilleri
Copy link
Member

According to Data.Binary's changelog, the IEEE-754 compliance was fixed in 0.8.4.0. (PR #119). So maybe it's worth testing option 3.

@krangelov
Copy link
Member

krangelov commented Nov 11, 2020 via email

@inariksit inariksit changed the title Is custom version of Data.Binary still needed? It uses accursedUnutterablePerformIO 😨 Is custom version of Data.Binary still needed? Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants