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 method to get a map entry by index #290

Merged
merged 2 commits into from
Jan 12, 2024
Merged

Conversation

bkragl
Copy link
Contributor

@bkragl bkragl commented Dec 11, 2023

This change adds a method get_index_entry to IndexMap, which is the by-index counterpart to entry. The returned struct IndexedEntry provides the same methods as OccupiedEntry, except replace_key.

A concrete use case where I found this useful is having a map: IndexMap<_, Vec<_>> with the invariant that each Vec is non-empty. Imagine we want to pick an entry at a random index and pop a value from the Vec. Without this change we'd have to use get_mut_index and swap_remove_index, and ensure that we use the same index. With get_index_entry we can write:

let mut e = map.get_index_entry(index).unwrap();
let v = e.get_mut();
let x = v.pop().unwrap();
if v.is_empty() {
    e.swap_remove();
}

Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

This does seem like a reasonable API to add, thanks!

src/map.rs Outdated
/// Valid indices are *0 <= index < self.len()*
///
/// Computes in **O(1)** time.
pub fn entry_at(&mut self, index: usize) -> Option<EntryAt<'_, K, V>> {
Copy link
Member

Choose a reason for hiding this comment

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

The name is okay, but just to bikeshed a little, what do you think about get_index_entry and IndexedEntry? I think that may fit a little better with get_index and get_index_mut, and "get" methods in general returning Option, as well as similarity to the other adjective-Entry types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dig IndexedEntry, and also renamed the method to get_index_entry. Some other options for the method would be entry_index (fits the existing entry method and get/get_index pattern, but feels odd) or indexed_entry (fits the return type). I don't have a strong opinion, but I'm happy to do another revision if somebody else has.

src/map/core.rs Outdated Show resolved Hide resolved
src/map/core.rs Outdated Show resolved Hide resolved
src/map/core.rs Outdated Show resolved Hide resolved
src/map/core.rs Outdated Show resolved Hide resolved
Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

Looks good!

src/map.rs Outdated
/// Valid indices are *0 <= index < self.len()*
///
/// Computes in **O(1)** time.
pub fn get_index_entry(&mut self, index: usize) -> Option<IndexedEntry<'_, K, V>> {
Copy link
Member

Choose a reason for hiding this comment

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

My last note is that this should move down near get_index because of the relaxed impl constraints there -- this does not (and will never) need K: Hash + Eq, S: BuildHasher. But I'll go ahead and move that myself, since you've allowed maintainer pushes.

@cuviper cuviper merged commit 84f772d into indexmap-rs:master Jan 12, 2024
14 checks passed
@bkragl bkragl deleted the entry_at branch January 28, 2024 14:13
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