Skip to content

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