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

added ArrayVecCopy #193

Closed
wants to merge 4 commits into from
Closed

added ArrayVecCopy #193

wants to merge 4 commits into from

Conversation

pYtoner
Copy link

@pYtoner pYtoner commented Jul 17, 2021

Made a copy of ArrayVec (ArrayVecCopy) that can only hold Copy items.

This means copying all of its relevant functions and relevant tests.

Closes #32

@bluss
Copy link
Owner

bluss commented Jul 19, 2021

Looks to be in the right direction, that's nice - I'll be a bit unreachable during the summer, but back in a bit

@bluss bluss added this to the 0.8 milestone Oct 28, 2021
@bluss
Copy link
Owner

bluss commented Nov 8, 2021

Ok great, ArrayVecImpl has made it possible to share some core code. Any other ideas for how to share more? This is quite a lot of code to duplicate, even of probably half of it is necessary.


impl<T: Copy, const CAP: usize> ExactSizeIterator for IntoIter<T, CAP> { }

impl<T: Copy, const CAP: usize> Drop for IntoIter<T, CAP> {
Copy link
Owner

Choose a reason for hiding this comment

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

This iterator doesn't need to implement Drop - because T is Copy.

}

/// A draining iterator for `ArrayVecCopy`.
pub struct Drain<'a, T: Copy + 'a, const CAP: usize> {
Copy link
Owner

Choose a reason for hiding this comment

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

Drain could be a candidate for sharing between the two implementations, even if it requires a bit of refactoring.

@@ -57,3 +59,6 @@ pub use crate::array_string::ArrayString;
pub use crate::errors::CapacityError;

pub use crate::arrayvec::{ArrayVec, IntoIter, Drain};

#[cfg(feature="copy")]
pub use crate::arrayvec_copy::ArrayVecCopy;
Copy link
Owner

Choose a reason for hiding this comment

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

There are other public items inside arrayvec_copy that we need to make reachable for the users - for example its iterator types. It's likely it needs to be in its own module, what do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, its own module would make using it easier.

/// The caller must ensure the length of the input fits in the capacity.
pub(crate) unsafe fn extend_from_iter<I, const CHECK: bool>(&mut self, iterable: I)
where I: IntoIterator<Item = T>
{
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe this method can move to the ArrayVecImpl trait?

@pYtoner
Copy link
Author

pYtoner commented Nov 9, 2021

Nice to see you are back. I will do the changes to make it better. This was more of a quick workaround for those needing this badly... I'll think of some ways to minimize code duplication too.

@pYtoner
Copy link
Author

pYtoner commented Nov 9, 2021

Doing this with a macro would be the easiest way to remove loads of code duplication. Is that something you think would be acceptable for this crate? (Some people don't like macros 😅)

@bluss
Copy link
Owner

bluss commented Nov 9, 2021

Hi. Please use rebase and not merge to update the PR. Sorry I've been away from github a lot. Macros are ok, I like them, also wrote some eyesore ones myself. Now it's hard to judge in every case, but let's say that we want method bodies to be easy to read. I wouldn't put a whole module's worth of code through 1 macro, maybe it can be per method (or group of smaller trait impls).

@pYtoner
Copy link
Author

pYtoner commented Nov 10, 2021

I am not sure what I did to my repo I am going to start over.

@bluss
Copy link
Owner

bluss commented Nov 16, 2021

Don't worry. I think you could have force-pushed to your PR branch if you wanted. Doesn't matter so much now.

@jacobsvante
Copy link

I would love to see this functionality in arrayvec. @pYtoner what was left to finish with this?

@pYtoner
Copy link
Author

pYtoner commented Feb 9, 2024

I had other stuff to do so I forgot about this @jacobsvante. :/ But I'll look into whats changed in the crate since then. Maybe this is a lot easier to implement now.

@pYtoner
Copy link
Author

pYtoner commented Feb 9, 2024

So the crate is pretty much the same as when I tried to do this PR. The problem was that I had skill issues with git..

Not sure if this crate is still maintained though but I'll fix my PR.

@jacobsvante
Copy link

That sounds lovely!

Let me know if you need some help with Git (though I'm not exactly a master myself!)

@jacobsvante
Copy link

@bluss Are you still maintaining this repo?

@pYtoner pYtoner mentioned this pull request Feb 9, 2024
@pYtoner
Copy link
Author

pYtoner commented Feb 9, 2024

@jacobsvante Alright I opened a new one: #261

@pYtoner pYtoner closed this Feb 9, 2024
@jacobsvante
Copy link

Thanks @pYtoner 😀

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.

Feature Request: ArrayVec that is Copy
3 participants