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

Why do copy_from_slice and clone_from_slice require equal size? #49769

Closed
RandomInsano opened this issue Apr 7, 2018 · 9 comments · Fixed by #51701
Closed

Why do copy_from_slice and clone_from_slice require equal size? #49769

RandomInsano opened this issue Apr 7, 2018 · 9 comments · Fixed by #51701
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@RandomInsano
Copy link
Contributor

RandomInsano commented Apr 7, 2018

Would you like to fix this issue? Here's how!


I'm dealing with a case where I have to send a byte array buffer to a destructive function multiple times. This function requires the buffer to start with an initialization sequence that I have to set.

I want to be able to efficiently reset the header bytes but the size of the initialization sequence is less than the total buffer and so I hit the destination and source slices have different lengths error.

The real question is, why is this required? Here's the current code:

    #[inline]
    fn copy_from_slice(&mut self, src: &[T]) where T: Copy {
        assert!(self.len() == src.len(),
                "destination and source slices have different lengths");
        unsafe {
            ptr::copy_nonoverlapping(
                src.as_ptr(), self.as_mut_ptr(), self.len());
        }
    }

In theory, if self is big enough to hold src shouldn't that be enough? The adjusted call should be this then:

    #[inline]
    fn copy_from_slice(&mut self, src: &[T]) where T: Copy {
        assert!(self.len() >= src.len(),
                "destination slice must be large enough to hold source");
        unsafe {
            ptr::copy_nonoverlapping(
                src.as_ptr(), self.as_mut_ptr(), src.len());
        }
    }

I think the same should be done for clone_from_slice.

@sfackler
Copy link
Member

sfackler commented Apr 8, 2018

The discussion of that decision is in #27750.

@RandomInsano
Copy link
Contributor Author

Holy bikeshed Batman! It’s impossible to find in all those comments! Thanks for pointing me there.

It does seem from the first 30 or so posts that it was intentionally meant to fill the destination. If it does stick around, I do think the size checking should be done a little smarter.

@scottmcm
Copy link
Member

Isn't buffer[..n].copy_from_slice(init_seq) fine? Personally, I find a bunch of value in the explicitness of requiring equal lengths since slicing to get the other three possible behaviours is so easy.

@RandomInsano
Copy link
Contributor Author

That's not a bad argument. Maybe we can adjust this to suggest that in the error message? I wouldn't have found that on my own.

@scottmcm
Copy link
Member

#46219 added some examples to the docs of using subslices with these methods. I don't know what the libs policy is on adding guidance to panics -- it does feel like one wouldn't want a long explanatory string built into one's binary in release mode, at least...

@RandomInsano
Copy link
Contributor Author

Agreed. In general, the longer the message the less likely people are going to read it clearly. I also believe not just stating the problem, but also offering a suggestion.

“Destination and source slices have different lengths. Try sub-slicing the longer of the two.”

This offers a jumping off point for Googling and reminds those (like me) that sub-slicing is a thing.

@Mark-Simulacrum
Copy link
Member

I'm personally against lengthening panic messages to suggest fixes, rather I'd expect someone to ask a question about this on StackOverflow or a similar platform eventually which would then get indexed by search engines. I'm going to mark this as an easy documentation issue; we can update documentation to note that subslicing is likely what users want to fix the panic in the panic section or the main section.

@Mark-Simulacrum Mark-Simulacrum added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools labels May 28, 2018
@steveklabnik
Copy link
Member

I'm happy to mentor anyone who wants to fix this issue! Here's how:

The relevant documentation lives here:

rust/src/libcore/slice/mod.rs

Lines 1598 to 1601 in e063518

/// # Panics
///
/// This function will panic if the two slices have different lengths.
///

This should be extended to describe how to use slicing to make the lengths the same. Something like the

let src = [1, 2, 3, 4];
let mut dst = [0, 0];

dst.copy_from_slice(&src[2..]);

assert_eq!(src, [1, 2, 3, 4]);
assert_eq!(dst, [3, 4]);

example, but with text pointing out how without the slice, it would panic, so slicing to the same length would make the guarantee.

I'm happy to answer any more questions about how to accomplish this!

@anirudhb
Copy link
Contributor

PR: #51701

@XAMPPRocky XAMPPRocky added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 7, 2018
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jul 11, 2018
Better docs for copy_from_slice & clone_from_slice

I copy-pasted the text from clone_from_slice to copy_from_slice 😄

@steveklabnik feel free to suggest changes.

edit: closes rust-lang#49769
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants