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

Remove DeepClone #12698

Closed
brson opened this issue Mar 5, 2014 · 5 comments · Fixed by #12706
Closed

Remove DeepClone #12698

brson opened this issue Mar 5, 2014 · 5 comments · Fixed by #12706
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@brson
Copy link
Contributor

brson commented Mar 5, 2014

It has no users and very few if any use cases. Having it is confusing.

@thestinger
Copy link
Contributor

I think this would have use cases if it was clever enough to handle cycles in Gc<T> graphs. It's very hard to clone an object graph with cycles by hand, and often impossible due to privacy.

@pongad
Copy link
Contributor

pongad commented Mar 5, 2014

I'll work on it. @thestinger If you have a complex structure, shouldn't you just do deep clone on clone()? My gut feeling is that it's difficult to make a useful shallow clone on things with cycles.

@thestinger
Copy link
Contributor

@pongad: Clone doesn't do a deep clone. It's defined as doing the minimum possible work to produce T from &T. For example, with Rc<T> it performs a reference count. In contrast, DeepClone allocates a new Rc<T> box and does a deep_clone() of the contained T.

I have no doubt that it's useful to provide deep clones for Rc<T> and removing the methods is a loss of useful functionality. Whether it should be a trait is another question, but it's certainly useful to be able to derive the DeepClone implementation for a struct or enum containing Rc<T>.

I don't see a reason to force users to write Rc::new(box.borrow().clone()) and leave full deep clones as an open problem to be solved manually. We don't force people to write ~((*x).clone())...

@thestinger
Copy link
Contributor

A shallow clone is just as easy to make for an object with cycles... deep clones are the difficult operation.

@thestinger
Copy link
Contributor

If anything, a good argument for removing it would be that it cannot currently work for cyclic Gc<T> graphs, but in my opinion it would be better to come up with a full fix instead.

@bors bors closed this as completed in 5f64adc Mar 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants