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

Deprecate insertBehind #96

Merged
merged 1 commit into from
Mar 27, 2023
Merged

Conversation

treeowl
Copy link
Collaborator

@treeowl treeowl commented Mar 27, 2023

insertBehind does not interact in any particularly sensible way with merges. I don't have a lot of faith that it works in other cases at all—it's far too fragile. It's also too slow to be really useful.

First stage in #35

@treeowl treeowl requested a review from konsumlamm March 27, 2023 01:12
Copy link
Collaborator

@konsumlamm konsumlamm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me. It's weird to have a function that makes guarantees about the order of elements with equal keys, when all the other functions don't. I guess in most cases insert would suffice. At least no package on Hackage seems to use it (https://hackage-search.serokell.io/?q=insertBehind). I'm reluctant to entirely remove it any time soon though.

@treeowl
Copy link
Collaborator Author

treeowl commented Mar 27, 2023

I'm reluctant to entirely remove it any time soon though.

We've never had any tests to demonstrate that it works. Those tests will not be easy, since other operations in the module could potentially mess up the relative ordering. I don't want to support this function, and I want it gone ASAP. It belongs in a different data structure altogether—one with two "fingers" rather than one.

@treeowl
Copy link
Collaborator Author

treeowl commented Mar 27, 2023

To clarify what I said in my last comment, that one function puts a major maintenance burden on other functions (especially insert and minViewWithKey) without contributing much on its own. It also has no specified (or obvious) merge semantics, so it can't be used usefully with merges (which are the raison d'être of binomial queues). It's very expensive dead weight, and we don't even know if we've met the heavy maintenance burden.

`insertBehind` does not interact in any particularly sensible way with
merges. I don't have a lot of faith that it works in other cases at
all—it's far too fragile. It's also too slow to be really useful.

First stage in lspitzner#35
@treeowl treeowl force-pushed the deprecate-insertBehind branch from 375eafd to 2f81e34 Compare March 27, 2023 07:27
@treeowl treeowl merged commit 34d93b6 into lspitzner:master Mar 27, 2023
@konsumlamm
Copy link
Collaborator

I think we don't have to consider deprecated functions too much when changing other functions. Just provide it as "it's there, maybe it works". I'm not against removing it at all, just not that soon.

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