-
Notifications
You must be signed in to change notification settings - Fork 88
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
Make Eq and Ord foreign functions less JS-specific #183
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.
Thanks @andyarvanitis, this looks great.
I'm happy for unsafeCompare
to die; it does make this a breaking change.
Did a quick search on GitHub but didn't find any proper usages.
I doubt it's very commonly used, and it's easy enough to fix.
Is this blocked on anything? |
@bb010g Not that I'm aware of. Maybe @LiamGoodacre knows? |
It’s mostly because a breaking change to Prelude has a big knock on effect on essentially the entire ecosystem, forcing more or less everyone to update their package manifest files. Therefore we tend to leave any breaking changes sitting in PRs for a while until we have a few we need to make, and do them all together to reduce the frequency of major version bumps. Come to think of it I think we should keep unsafeCompare in for the time being but add a deprecation warning; that way we can get this merged sooner. |
Sounds good; I'll add |
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.
Thanks!
While working on updating the C++ backend (now using dump-corefn), I found these foreign function implementations to be problematic. A typed backend (combined with type erasure) doesn't work with the assumptions these make. Note the removal of
Data.Ord.Unsafe.unsafeCompare
-- I don't think anyone else would need it(?), but if so, it can be put back.