-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add a resource-reusing method to ToOwned
#41009
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BurntSushi (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
Odd, that error didn't show up from |
That tidy lint landed recently, so your local git might not have it yet. |
cc @rust-lang/libs |
☔ The latest upstream changes (presumably #41102) made this pull request unmergeable. Please resolve the merge conflicts. |
f56ca91
to
755d0ad
Compare
Rebased; looks like it's applying cleanly again. |
Seems like a reasonable addition to me! |
src/libcollections/borrow.rs
Outdated
/// let mut v: Vec<i32> = Vec::new(); | ||
/// [1, 2][..].clone_into(&mut v); | ||
/// ``` | ||
#[unstable(feature = "toowned_clone_into", issue = "123456789")] // FIXME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the FIXME here for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue # I'm assuming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To remind me to put a tracking issue in. If this has a plausible chance of acceptance, I can go create a placeholder now and put a real number.
Discussed at libs triage the conclusion was to merge! @scottmcm want to update the tracking issue and I'll r+? |
to_owned generalizes clone; this generalizes clone_from. Use to_owned to give it a default impl. Customize the impl for [T], str, and T:Clone. Use it in Cow::clone_from to reuse resources when cloning Owned into Owned.
755d0ad
to
7ec27ae
Compare
Added tracking issue #41263 and squashed down to a single commit. |
📌 Commit 7ec27ae has been approved by |
Add a resource-reusing method to `ToOwned` `ToOwned::to_owned` generalizes `Clone::clone`, but `ToOwned` doesn't have an equivalent to `Clone::clone_from`. This PR adds such a method as `clone_into` under a new unstable feature `toowned_clone_into`. Analogous to `clone_from`, this has the obvious default implementation in terms of `to_owned`. I've updated the `libcollections` impls: for `T:Clone` it uses `clone_from`, for `[T]` I moved the code from `Vec::clone_from` and implemented that in terms of this, and for `str` it's a predictable implementation in terms of `[u8]`. Used it in `Cow::clone_from` to reuse resources when both are `Cow::Owned`, and added a test that `Cow<str>` thus keeps capacity in `clone_from` in that situation. The obvious question: is this the right place for the method? - It's here so it lives next to `to_owned`, making the default implementation reasonable, and avoiding another trait. But allowing method syntax forces a name like `clone_into`, rather than something more consistent like `owned_from`. - Another trait would allow `owned_from` and could support multiple owning types per borrow type. But it'd be another single-method trait that generalizes `Clone`, and I don't know how to give it a default impl in terms of `ToOwned::to_owned`, since a blanket would mean overlapping impls problems. I did it this way as it's simpler and many of the `Borrow`s/`AsRef`s don't make sense with `owned_from` anyway (`[T;1]:Borrow<[T]>`, `Arc<T>:Borrow<T>`, `String:AsRef<OsStr>`...). I'd be happy to re-do it the other way, though, if someone has a good solution for the default handling. (I can also update with `CStr`, `OsStr`, and `Path` once a direction is decided.)
☀️ Test successful - status-appveyor, status-travis |
…crichton Override ToOwned::clone_into for Path and OsStr The only non-overridden one remaining is the CStr impl, which cannot be optimized as doing so would break CString's second invariant. Follow-up to 7ec27ae (PR rust-lang#41009). r? @alexcrichton
…crichton Override ToOwned::clone_into for Path and OsStr The only non-overridden one remaining is the CStr impl, which cannot be optimized as doing so would break CString's second invariant. Follow-up to 7ec27ae (PR rust-lang#41009). r? @alexcrichton
ToOwned::to_owned
generalizesClone::clone
, butToOwned
doesn't have an equivalent toClone::clone_from
. This PR adds such a method asclone_into
under a new unstable featuretoowned_clone_into
.Analogous to
clone_from
, this has the obvious default implementation in terms ofto_owned
. I've updated thelibcollections
impls: forT:Clone
it usesclone_from
, for[T]
I moved the code fromVec::clone_from
and implemented that in terms of this, and forstr
it's a predictable implementation in terms of[u8]
.Used it in
Cow::clone_from
to reuse resources when both areCow::Owned
, and added a test thatCow<str>
thus keeps capacity inclone_from
in that situation.The obvious question: is this the right place for the method?
to_owned
, making the default implementation reasonable, and avoiding another trait. But allowing method syntax forces a name likeclone_into
, rather than something more consistent likeowned_from
.owned_from
and could support multiple owning types per borrow type. But it'd be another single-method trait that generalizesClone
, and I don't know how to give it a default impl in terms ofToOwned::to_owned
, since a blanket would mean overlapping impls problems.I did it this way as it's simpler and many of the
Borrow
s/AsRef
s don't make sense withowned_from
anyway ([T;1]:Borrow<[T]>
,Arc<T>:Borrow<T>
,String:AsRef<OsStr>
...). I'd be happy to re-do it the other way, though, if someone has a good solution for the default handling.(I can also update with
CStr
,OsStr
, andPath
once a direction is decided.)