Skip to content

Use Data.Ord.Down #93

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Use Data.Ord.Down #93

wants to merge 1 commit into from

Conversation

treeowl
Copy link
Collaborator

@treeowl treeowl commented Mar 17, 2023

We used to use our own Down newtype because the one in Data.Ord had a somewhat lousy Ord instance. That has since been corrected, so we can just do what everyone else does.

@treeowl treeowl requested a review from konsumlamm March 17, 2023 04:58
We used to use our own `Down` newtype because the one in `Data.Ord`
had a somewhat lousy `Ord` instance. That has since been corrected, so
we can just do what everyone else does.
@treeowl
Copy link
Collaborator Author

treeowl commented Mar 17, 2023

Hmmm.... That's a lot of mess still. We presumably don't want to expose orphan instances from base-orphans. For some things (like Foldable) we could do the gunk by hand. For others, like Data, I imagine the best we can do is some FlexibleContexts junk with constraints like Data (Down a), letting downstream pull in base-orphans. Not the prettiest situation at all. Perhaps the whole Down swap should go on the back burner for a few more years?

@konsumlamm
Copy link
Collaborator

Perhaps the whole Down swap should go on the back burner for a few more years?

Yeah, I think that's the best option for now. Having constraints on Down leaks internal details (the users shouldn't have to care about wether we use Down or something else) and implementing the instances by hand defeats the whole point of this change (namely to reduce code size). Using base-orphans would be an option, but that also pulls in a whole bunch of other instances (that might even conflict with some local orphan instances), so not a great option.

Let's also link #56, as that's the issue for this.

One thing I did notice though, we can probably replace a bunch of the fmap Down and fmap unDown by coerce, since Down is a newtype (Down and unDown too, but that shouldn't make a difference). We should also add benchmarks for max queues, to examine their overhead over min queues.

@treeowl treeowl marked this pull request as draft March 17, 2023 18:45
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.

2 participants