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

util: add JoinMap::keys #6046

Merged
merged 3 commits into from
Oct 15, 2023
Merged

util: add JoinMap::keys #6046

merged 3 commits into from
Oct 15, 2023

Conversation

andreastedile
Copy link
Contributor

In a library I am developing, I need to know what tasks are contained in a JoinMap.

@Darksonn Darksonn added A-tokio-util Area: The tokio-util crate M-task Module: tokio/task labels Oct 3, 2023
/// call to [`join_next`], this method will still return its key.
///
/// [`join_next`]: fn@Self::join_next
pub fn keys<Q: ?Sized>(&self) -> impl Iterator<Item = &K>
Copy link
Member

Choose a reason for hiding this comment

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

it's definitely much simpler to return an impl Iterator here. However, I will note that the hash_map::Keys iterator that this wraps also implements some other traits: in addition to being an Iterator, it also implements Clone, ExactSizeIterator, and FusedIterator. It might be better if we returned an iterator that also implements those traits.

In particular, ExactSizeIterator is used by the standard library to implement optimizations when collect()ing an iterator into particular collections, like Vecs and HashMaps, since the size of the iterator can be reserved up front by with_capacity, instead of allocating multiple times while collecting.

Therefore, we may want to change this to return a named Keys iterator, like this:

use std::iterator::{ExactSizeIterator, FusedIterator};

pub struct Keys<'map, K, S = RandomState> {
    inner: hashbrown::hash_map::Keys<'map, Key<K>, AbortHandle, S>>,
}

impl<'map, K, S> Iterator for Keys<'map, K, S> {
    type Item = &'map K;

    fn next(&mut self) -> Option<Self::Item> {
         self.inner.next.map(|key| &key.key)
    }

    fn size_hint(&self) -> (usize, Option<usize>) {
        self.inner.size_hint()    
    }
}

impl<'map, K, S> ExactSizeIterator for Keys<'map, K, S> {
    fn len(&self) -> usize {
         self.inner.len()
    }

    fn is_empty(&self) -> bool {
         self.inner.is_empty()
    }
}

impl<'map, K, S> FusedIterator for Keys<'map, K, S> {}

Or, alternatively, I think we could write

Suggested change
pub fn keys<Q: ?Sized>(&self) -> impl Iterator<Item = &K>
pub fn keys<Q: ?Sized>(&self) -> impl Iterator<Item = &K> + ExactSizeIterator + FusedIterator

but I think the named type might be a nicer API surface for users?

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. I made a few changes according to Eliza's review.

@andreastedile Since it will be attributed to you, please let me know whether you are okay with me merging the current contents of this PR.

@andreastedile
Copy link
Contributor Author

@Darksonn Yes, I am okay with it: I agree with the suggestions and modifications. Thank you both for dedicating time to this!

@Darksonn Darksonn merged commit f1e41a4 into tokio-rs:master Oct 15, 2023
69 checks passed
@Darksonn
Copy link
Contributor

I'll make a release containing this once #6033 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-util Area: The tokio-util crate M-task Module: tokio/task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants