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

Implement Clone, Debug for iterators #543

Closed
wants to merge 1 commit into from

Conversation

clarfonthey
Copy link
Contributor

Follow-up to #542 after I noticed these were missing too. A more complete implementation of #541.

Adds:

  • explicit Clone for raw::Iter and raw::IntoIter
  • derived Clone for iterators that use raw::IntoIter internally
  • explicit Debug implementations for raw::Iter, raw::IterMut, and raw::IntoIter
  • public iter and iter_mut methods to raw::IntoIter (only iter is needed for Debug, but I decided to add these as public since map and set IntoIter seem to have public iter methods)

@@ -4280,6 +4280,7 @@ impl ExactSizeIterator for FullBucketsIndices {}
impl FusedIterator for FullBucketsIndices {}

/// Iterator which consumes a table and returns elements.
#[derive(Clone)]
pub struct RawIntoIter<T, A: Allocator = Global> {
iter: RawIter<T>,
allocation: Option<(NonNull<u8>, Layout, A)>,
Copy link
Member

Choose a reason for hiding this comment

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

This is not going to work correctly since it will end up double-freeing the underlying allocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, that makes sense.

I'll look into actually cloning the allocation instead.

@clarfonthey
Copy link
Contributor Author

clarfonthey commented Aug 18, 2024

Going to close this per: rust-lang/rust#128711 (comment)

I have some ideas on a refactor that would make implementing Clone a bit more comfortable, and I'll draft up my general plans in an issue first before implementing so folks can comment on them.

I posted some less constructive feedback earlier today, but the long story short is that I think that there need to be more safe wrappers on the internals in the code, rather than every single usage of the raw table being unsafe. I need to do more reading into the code before I make a proper writeup though, so, stay tuned.


Also poked through the currently filed issues just to check for prior art before I start this, and #290 feels relevant here as well. Will want to link that in the issue I file. (Note to self.)

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