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

improve clone impl as per https://github.com/Lokathor/tinyvec/issues/143 #144

Merged
merged 5 commits into from
Jul 22, 2021

Conversation

Lokathor
Copy link
Owner

@Lokathor Lokathor commented Jul 6, 2021

@Lokathor Lokathor requested a review from Nemo157 July 6, 2021 22:53
@Shnatsel
Copy link
Collaborator

Shnatsel commented Jul 6, 2021

Wait, isn't clone_from supposed to discard the data that was previously contained by the collection? Why is there zip then? Nevermind I'm dumb

@Shnatsel
Copy link
Collaborator

Shnatsel commented Jul 6, 2021

I wonder if a fast path for Copy types is necessary, for cases like a long-ish vec of u64? I'm not sure the current implementation would lower to a memcpy()

Edit: something like Rgb struct would be more egregious that has several Copy types in it

@Lokathor
Copy link
Owner Author

Lokathor commented Jul 7, 2021

Best as I understand, this is improving the situation in some but not all situations. If this code is bad for a particular copy type, then it was previously also bad for that type. EDIT: but i'm not generally familiar with when clone_from vs clone is called.

for (dst, src) in iter {
dst.clone_from(src)
}
self.len = o.len;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might want to take-and-drop all the elements from o.len..self.len.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just remember to skip this part if o.len >= self.len

Copy link
Owner Author

Choose a reason for hiding this comment

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

If I follow you right, we'd want to have something like:

if o.len < self.len {
self[o.len..].iter_mut().for_each(|r| take(r));
}

immediately after the for loop?

Copy link
Contributor

Choose a reason for hiding this comment

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

if let Some(to_drop) = self.data.get_mut(o.len() .. self.len()) {
  to_drop.for_each(|x| take(x));
}

Lokathor added 4 commits July 21, 2021 18:03
we don't have deref for the A so we have to keep using as_slice_mut and such
@Lokathor Lokathor merged commit 1300873 into main Jul 22, 2021
@Lokathor Lokathor deleted the improve-clone branch July 22, 2021 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TinyVec/ArrayVec's Clone uses the naive (default) version of Clone::clone_from.
4 participants