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

Add Map::remove_entry #708

Closed
wants to merge 3 commits into from
Closed

Conversation

jplatte
Copy link
Contributor

@jplatte jplatte commented Sep 18, 2020

… and simplify Map::remove, since I noticed it uses #[cfg] branching when it doesn't need to.

@jplatte
Copy link
Contributor Author

jplatte commented Sep 18, 2020

Okay so BTreeMap::remove_entry is only available since 1.45.0. Would feature-gating stuff like this that requires a higher MSRV be an option?

IndexMap has a remove method that is the same as swap_remove, so there
is no need to use #[cfg] branching for this.
Which simply calls the same method on the underlying map.
@jplatte
Copy link
Contributor Author

jplatte commented Sep 30, 2020

@dtolnay It's now behind a feature flag that is also tested on CI. Can you take a look?

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I would prefer not to do feature stuff for this. It would be better to emulate the API if remove_entry is not directly available on the underlying type. Is that possible?

@jplatte
Copy link
Contributor Author

jplatte commented Sep 30, 2020

No, unfortunately I don't think that is possible. The entry API requires taking ownership of the key which defeats the whole point, and a raw entry API is only available for HashMap, not BTreeMap.

@dtolnay
Copy link
Member

dtolnay commented Sep 30, 2020

Closing in favor of #712 which emulates the API when remove_entry is not available on the underlying type.

@dtolnay dtolnay closed this Sep 30, 2020
@jplatte jplatte deleted the map-remove-entry branch September 30, 2020 21:19
@jplatte
Copy link
Contributor Author

jplatte commented Sep 30, 2020

I see, so using a less efficient implementation on older versions. I like that 👍

@jplatte jplatte restored the map-remove-entry branch October 2, 2020 16:14
@jplatte jplatte deleted the map-remove-entry branch October 2, 2020 16:14
@jplatte jplatte mentioned this pull request Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants