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

Consider implementing entries(&mut self) -> impl Iterator<Item = OccupiedEntryRef> #426

Open
JakobDegen opened this issue Apr 24, 2023 · 7 comments

Comments

@JakobDegen
Copy link

See my comment here for the motivation: rust-lang/rust#59618 (comment) .

Unfortunately, the impl can't just be added. hashbrown's EntryRefs are currently Send, which means that they can't be allowed to co-exist. Allowing this API would require removing the Send impls on those types. Before someone starts with the implementation, is that something the crate would consider? The tradeoff is pretty clearly well-motivated imo, especially since the entry types already have lifetimes and so aren't exactly likely to be sent across threads much.

@Amanieu
Copy link
Member

Amanieu commented Apr 25, 2023

That doesn't work due to the way the Iterator trait works: the value returned by next() is not tied to the lifetime of the iterator, which means you can call it multiple times and end up with multiple OccupiedEntryRef at the same time. This is unsound since they each hold a mutable reference to the underlying HashMap.

@JakobDegen
Copy link
Author

@Amanieu sure, but that's fixable by just... Not doing that, right? We can have them hold a *mut HashMap and a pointer to their bucket or something like that

@Amanieu
Copy link
Member

Amanieu commented Apr 25, 2023

The problem is in the Item type on the Iterator trait: it has to contain a lifetime but this lifetime cannot refer to the lifetime of the iterator itself. Consider IterMut of Vec for example, it returns &'a mut T but you can call .next mutliple times on it to obtain multiple mutable references to distinct elements at the same time.

@JakobDegen
Copy link
Author

I understand that. My point is that we should allow OccupiedEntryRefs to coexist - that's a feature. I don't think there's any reason that can't be implemented outside of needing to make them non-Send

@Amanieu
Copy link
Member

Amanieu commented Apr 25, 2023

This exposes too much of the implementation details of the current hash table implementation. For example, the previous implementation (before hashbrown) would shift adjacent entries on deletion. This would invalidate references to other elements in the table.

@JakobDegen
Copy link
Author

Is that comment from the pov of std or the pov of hashbrown? Putting std aside for a minute, would hashbrown consider this API?

@Amanieu
Copy link
Member

Amanieu commented Apr 25, 2023

No, I would like to keep the high-level HashMap/HashSet independent of the underlying implementation. Of course this is not the case for RawTable which directly exposes implementation details.

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

No branches or pull requests

2 participants