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

Add Vec::drain and String::drain. #444

Merged
merged 2 commits into from
Jun 30, 2024
Merged

Conversation

reitermarkus
Copy link
Member

Closes #334 and #365.

@Dirbaio
Copy link
Member

Dirbaio commented Jan 23, 2024

could you add tests?

src/vec/drain.rs Outdated
pub(super) tail_len: usize,
/// Current remaining range to remove
pub(super) iter: slice::Iter<'a, T>,
pub(super) vec: NonNull<Vec<T, N>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

If this was built on top of #425 this would allow removing the const N: usize generic, and replace the pointer with NonNull<VecView<T>>. #425 does not offer the View functionality for String to keep the PR small, but it would be easy to add it.

It is worth considering because merging this PR and releasing before merging #425 would make it a breaking change to remove the const generic, so there would need to be 2 implementation of drain, one for the view and one for the owned type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I agree it makes sense to base this on VecView instead.

@reitermarkus
Copy link
Member Author

I can't quite figure out the miri error. Note that the implementation is almost 1:1 copied from std, so I'm not sure if it's a false positive.

@Dirbaio
Copy link
Member

Dirbaio commented Jun 30, 2024

rebased it for you on top of the new VecView impl.

@Dirbaio Dirbaio added this pull request to the merge queue Jun 30, 2024
Merged via the queue into rust-embedded:main with commit 39c379c Jun 30, 2024
22 checks passed
@reitermarkus
Copy link
Member Author

@Dirbaio, thanks!

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.

Add method drain for Vec, like std::vec::Vec?
3 participants