-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
clone_from in the standard library #28481
Comments
Seems pretty reasonable to me. We might also want to consider updating deriving logic to implement clone_from explicitly |
cc #27939 |
My thoughts here are that one-off specialized implementations are fine, but I'm not a huge fan of altering |
|
Edited the list to add Option and Result |
Triage: the above list is still accurate today. |
Seems like a good issue for people who want to work on libstd |
Implement Clone::clone_from for Option and Result See #28481
…odrAus Implement Clone::clone_from for Option and Result See rust-lang#28481
@apasel422 You can update your comment to check Option and Result. :) |
Implement Clone::clone_from for LinkedList See rust-lang#28481. This represents a substantial speedup when the list sizes are comparable, and shouldn't ever be significantly worse. Technically split_off is doing an unnecessary search, but the code is hopefully cleaner as a result. I'm happy to rework anything that needs to be changed as well!
Implement Clone::clone_from for LinkedList See rust-lang#28481. This represents a substantial speedup when the list sizes are comparable, and shouldn't ever be significantly worse. Technically split_off is doing an unnecessary search, but the code is hopefully cleaner as a result. I'm happy to rework anything that needs to be changed as well!
Implement Clone::clone_from for LinkedList See rust-lang#28481. This represents a substantial speedup when the list sizes are comparable, and shouldn't ever be significantly worse. Technically split_off is doing an unnecessary search, but the code is hopefully cleaner as a result. I'm happy to rework anything that needs to be changed as well!
Do we currently have any specific requirements to field drop order for this method? The docs are too vague. This can actuallly cause problems. For example, the following code: #[derive(Clone)]
struct A;
impl Drop for A {
fn drop(&mut self) {
println!("A dropped");
}
}
#[derive(Clone)]
struct B;
impl Drop for B {
fn drop(&mut self) {
println!("B dropped");
}
}
struct Pair {
a: A,
b: B,
}
impl Clone for Pair {
fn clone(&self) -> Self {
Pair {
a: self.a.clone(),
b: self.b.clone(),
}
}
// fn clone_from(&mut self, other: &Self) {
// self.b = other.b.clone(); // <-- note: assigned in other order
// self.a = other.a.clone();
// }
}
fn main() {
let mut first = Pair { a: A, b: B };
let second = Pair { a: A, b: B };
first.clone_from(&second);
} prints:
, but if we uncomment custom implementation of
This can break the code which relies on specific drop order, so implementing |
Implement Clone::clone_from for VecDeque See rust-lang#28481. For simple data types with the target much longer than the source, this implementation can be significantly slower than the default (probably due to the use of truncate). However, it should be substantially faster when cloning from nested data structures with similar shapes or when cloning from VecDeques with similar lengths, hopefully more common use cases for clone_from.
Implement clone_from for BTreeMap and BTreeSet See #28481. This results in up to 90% speedups on simple data types when `self` and `other` are the same size, and is generally comparable or faster. Some concerns: 1. This implementation requires an `Ord` bound on the `Clone` implementation for `BTreeMap` and `BTreeSet`. Since these structs can only be created externally for keys with `Ord` implemented, this should be fine? If not, there's certainly a less safe way to do this. 2. Changing `next_unchecked` on `RangeMut` to return mutable key references allows for replacing the entire overlapping portion of both maps without changing the external interface in any way. However, if `clone_from` fails it can leave the `BTreeMap` in an invalid state, which might be unacceptable. This probably needs an FCP since it changes a trait bound, but (as far as I know?) that change cannot break any external code.
Implement clone_from for BTreeMap and BTreeSet See rust-lang#28481. This results in up to 90% speedups on simple data types when `self` and `other` are the same size, and is generally comparable or faster. Some concerns: 1. This implementation requires an `Ord` bound on the `Clone` implementation for `BTreeMap` and `BTreeSet`. Since these structs can only be created externally for keys with `Ord` implemented, this should be fine? If not, there's certainly a less safe way to do this. 2. Changing `next_unchecked` on `RangeMut` to return mutable key references allows for replacing the entire overlapping portion of both maps without changing the external interface in any way. However, if `clone_from` fails it can leave the `BTreeMap` in an invalid state, which might be unacceptable. ~This probably needs an FCP since it changes a trait bound, but (as far as I know?) that change cannot break any external code.~
@apasel422 you can check |
Nice catch! It should be pretty simple to resolve though, would you like to create a PR? |
Opened #80400. |
The
Clone
trait has a sadly-neglectedclone_from
method that clones a value into an existing one. There is a default implementation, but it's fairly naive and therefore not optimal for all cases, notably collections.It would be good if the major collections had better implementations for
clone_from
where it can be implemented more efficiently./cc @rust-lang/libs
The text was updated successfully, but these errors were encountered: