-
Notifications
You must be signed in to change notification settings - Fork 292
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
Implement Clone and Debug for HashTable's Iter struct #541
Conversation
src/table.rs
Outdated
@@ -1914,6 +1931,23 @@ impl<T> ExactSizeIterator for IterMut<'_, T> { | |||
|
|||
impl<T> FusedIterator for IterMut<'_, T> {} | |||
|
|||
// FIXME(#26925) Remove in favor of `#[derive(Clone)]` | |||
impl<'a, T> Clone for IterMut<'a, T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cloning a mutable iterator is unsound since it could be used to create multiple aliasing mutable references. All the other impls seem fine though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm still new to Rust. I understand your point and it makes sense, so I removed the implementation of Clone here. And in fact, looking at my own code, my wrapping iterators haven't been implementing this either.
However, if this leads to unsoundness, how come I was able to implement the logic in safe code and the compiler didn't complain? Should RawIter::clone be marked as unsafe so that what I did would flag a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't entirely safe: if you look above, the Iterator
trait implementation for IterMut
uses unsafe code. That unsafe code is only valid if it is the only instance of IterMut
for a table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't resolved, it seem you have re-added the incorrect Clone
impl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, redeleted. Sorry about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, Debug for IterMut
is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To implement Debug, I needed Clone, so I just punted on IterMut alltogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The map IterMut
does it through a private iter()
that only clones the inner: RawIter
.
Lines 3175 to 3183 in a69af93
impl<K, V> fmt::Debug for IterMut<'_, K, V> | |
where | |
K: fmt::Debug, | |
V: fmt::Debug, | |
{ | |
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | |
f.debug_list().entries(self.iter()).finish() | |
} | |
} |
Lines 2137 to 2146 in a69af93
impl<K, V> IterMut<'_, K, V> { | |
/// Returns a iterator of references over the remaining items. | |
#[cfg_attr(feature = "inline-more", inline)] | |
pub(super) fn iter(&self) -> Iter<'_, K, V> { | |
Iter { | |
inner: self.inner.clone(), | |
marker: PhantomData, | |
} | |
} | |
} |
☔ The latest upstream changes (presumably #549) made this pull request unmergeable. Please resolve the merge conflicts. |
29df316
to
d6a46a9
Compare
d6a46a9
to
e9fd7b4
Compare
@bors r+ |
☀️ Test successful - checks-actions |
Found those implementations missing, which is preventing me from implementing the same in encapsulating iterators.