Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add more Vec/slice-like methods to maps and sets #160

Merged
merged 3 commits into from
Dec 15, 2020

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Sep 30, 2020

impl<K, V, S> IndexMap<K, V, S> {
    pub fn truncate(&mut self, len: usize);
    pub fn split_off(&mut self, at: usize) -> Self where S: Clone;
    pub fn first(&self) -> Option<(&K, &V)>;
    pub fn first_mut(&mut self) -> Option<(&mut K, &mut V)>;
    pub fn last(&self) -> Option<(&K, &V)>;
    pub fn last_mut(&mut self) -> Option<(&mut K, &mut V)>;
    pub fn swap_indices(&mut self, a: usize, b: usize);
}

impl<T, S> IndexSet<T, S> {
    pub fn truncate(&mut self, len: usize);
    pub fn split_off(&mut self, at: usize) -> Self where S: Clone;
    pub fn first(&self) -> Option<&T>;
    pub fn last(&self) -> Option<&T>;
    pub fn swap_indices(&mut self, a: usize, b: usize);
}

I would prefer &K instead of &mut K in IndexMap::first_mut and
last_mut, but this is consistent with the existing get_index_mut.

@bluss
Copy link
Member

bluss commented Dec 2, 2020

get_index_mut seems to be an oversight. Mutable key access is supposedly hidden away in the trait MutableKeys and I think it seems ok to continue that.

src/map.rs Outdated
self.as_entries().first().map(Bucket::refs)
}

pub fn first_mut(&mut self) -> Option<(&mut K, &mut V)> {
Copy link
Member

@bluss bluss Dec 2, 2020

Choose a reason for hiding this comment

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

If you agree I'd do &K here, that's the initial interface we were going for in this crate (while still allowing opt-in access to mutable keys with the trait). Implementing a such "first_mut2" in MutableKeys could be done on a YAGNI basis 🙂

Copy link
Member

@bluss bluss left a comment

Choose a reason for hiding this comment

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

Comment about &K applies

Copy link
Member

@bluss bluss left a comment

Choose a reason for hiding this comment

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

I guess I technically can't approve while there are still methods without doc comment, but I like the idea.

@cuviper
Copy link
Member Author

cuviper commented Dec 2, 2020

OK, I added more docs for the public methods.

At some point we should probably go for #![deny(missing_docs)], because there are a lot more missing, but I don't want to force that into this PR.

@cuviper
Copy link
Member Author

cuviper commented Dec 2, 2020

Oh, I switched to &K too -- force-pushed to split up that commit.

@cuviper cuviper mentioned this pull request Dec 2, 2020
7 tasks
```rust
impl<K, V, S> IndexMap<K, V, S> {
    pub fn truncate(&mut self, len: usize);
    pub fn split_off(&mut self, at: usize) -> Self where S: Clone;
    pub fn first(&self) -> Option<(&K, &V)>;
    pub fn first_mut(&mut self) -> Option<(&mut K, &mut V)>;
    pub fn last(&self) -> Option<(&K, &V)>;
    pub fn last_mut(&mut self) -> Option<(&mut K, &mut V)>;
    pub fn swap_indices(&mut self, a: usize, b: usize);
}

impl<T, S> IndexSet<T, S> {
    pub fn truncate(&mut self, len: usize);
    pub fn split_off(&mut self, at: usize) -> Self where S: Clone;
    pub fn first(&self) -> Option<&T>;
    pub fn last(&self) -> Option<&T>;
    pub fn swap_indices(&mut self, a: usize, b: usize);
}
```

I would prefer `&K` instead of `&mut K` in `IndexMap::first_mut` and
`last_mut`, but this is consistent with the existing `get_index_mut`.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants