-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Document overrides of clone_from()
in core/std
#122201
Conversation
Specifically, when an override doesn't just forward to an inner type, document the behavior and that it's preferred over simply assigning a clone of source. Also, change instances where the second parameter is "other" to "source".
clone_from()
clone_from()
in core/std
r? libs-api |
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.
Thank you!
/// as it avoids reallocation if possible. Additionally, if the element type | ||
/// `T` overrides `clone_from()`, this will reuse the resources of `self`'s | ||
/// elements as well. |
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.
It doesn't do this currently. All existing elements are dropped.
@rust-lang/libs-api: Please take a skim through the added documentation and decide whether these are all commitments we can make. Some of them are in the form of "This method is preferred over simply assigning Other ones are stronger commitments. Deliberate new commitments:
"Commitments":
@rfcbot fcp merge |
Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
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.
It seems to me "avoids allocations if possible" could also be interpreted to sound like a much more affirmative "we will do this if there is any possible interpretation of our data structure that will avoid allocations here". That may prove to be a bit of a peculiar thing to promise, especially when there's nothing stopping you from using clone_from
for cloning a larger data structure "into" a smaller one, which will force new allocations.
It is possible that we may wish to say "reduces allocations" instead, as reducing the number of allocations by some N can validly be 0.
I am most concerned about this in the case of the types like BinaryHeap, LinkedList, and the Map/Set types, which have a somewhat more ambiguous internal representation, and in some cases are direly under-loved.
library/core/src/cell.rs
Outdated
@@ -1276,8 +1276,8 @@ impl<T: Clone> Clone for RefCell<T> { | |||
/// Panics if `other` is currently mutably borrowed. |
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.
nit: parameter rename missed in the comment.
I'm unsure of how the FCP interacts with reviews - should I address the nits/review comments while the FCP is active? |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
Please address the review comments at your leisure. It doesn't make a difference whether that happens while the FCP is active or afterward, unless some team member's approval is contingent on seeing the resolution of some concern (usually with "@rfcbot concern"). |
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.
Thank you! This looks good to me.
I think this wording is all right, but I would be interested to evaluate a followup PR if someone wants to address #122201 (review). The reason I think the current wording in the PR is all right is I don't see "avoids reallocation if possible" being interpreted as "does not allocate if there is any possible interpretation of our data structure that could implement this operation without allocating". It instead just means "avoids those allocations which are possible to avoid per this manifestation of the data structure". The more interesting guarantees being committed to are the ones listed in #122201 (comment).
@bors r+ |
@bors ping |
@bors ping |
😪 I'm awake I'm awake |
@bors r+ |
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#116957 (meta: notify #t-rustdoc Zulip stream on backport nominations) - rust-lang#122201 (Document overrides of `clone_from()` in core/std) - rust-lang#122723 (Use same file permissions for ar_archive_writer as the LLVM archive writer) - rust-lang#124030 (interpret: pass MemoryKind to adjust_alloc_base_pointer) - rust-lang#124037 (Don't ascend into parent bodies when collecting stmts for possible return suggestion) - rust-lang#124049 (Stabilize `const_io_structs`) - rust-lang#124062 (Add another expression to weird-exprs.rs) - rust-lang#124066 (Don't error on subtyping of equal types) - rust-lang#124073 (Remove libc from rust_get_test_int uses) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#122201 - coolreader18:doc-clone_from, r=dtolnay Document overrides of `clone_from()` in core/std As mentioned in rust-lang#96979 (comment) Specifically, when an override doesn't just forward to an inner type, document the behavior and that it's preferred over simply assigning a clone of source. Also, change instances where the second parameter is "other" to "source". I reused some of the wording over and over for similar impls, but I'm not sure that the wording is actually *good*. Would appreciate feedback about that. Also, now some of these seem to provide pretty specific guarantees about behavior (e.g. will reuse the exact same allocation iff the len is the same), but I was basing it off of the docs for [`Box::clone_from`](https://doc.rust-lang.org/1.75.0/std/boxed/struct.Box.html#method.clone_from-1) - I'm not sure if providing those strong guarantees is actually good or not.
As mentioned in #96979 (comment)
Specifically, when an override doesn't just forward to an inner type, document the behavior and that it's preferred over simply assigning a clone of source. Also, change instances where the second parameter is "other" to "source".
I reused some of the wording over and over for similar impls, but I'm not sure that the wording is actually good. Would appreciate feedback about that.
Also, now some of these seem to provide pretty specific guarantees about behavior (e.g. will reuse the exact same allocation iff the len is the same), but I was basing it off of the docs for
Box::clone_from
- I'm not sure if providing those strong guarantees is actually good or not.