Document overrides of `clone_from()` in core/std #122201

coolreader18 · 2024-03-08T18:36:22Z

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.

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".

rustbot · 2024-03-08T18:36:30Z

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

joboet · 2024-03-09T09:34:14Z

r? libs-api

dtolnay

Thank you!

dtolnay · 2024-03-31T18:40:15Z

library/alloc/src/collections/vec_deque/mod.rs

+ /// 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.


It doesn't do this currently. All existing elements are dropped.

dtolnay · 2024-03-31T19:03:14Z

@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 source.clone() to self, as it avoids reallocation if possible", and it's debatable to what extent that constitutes a commitment. It sounds like something that can be changed saying "yeah we decided it wasn't possible after all".

Other ones are stronger commitments.

Deliberate new commitments:

Box::<[T], A>::clone_from (equivalent to the already existing commitment on Box::<T, A>::clone_from for T: Sized)
LinkedList::<T, A>::clone_from
Vec::<T, A>::clone_from
Waker::clone_from (Override Waker::clone_from to avoid cloning Wakers unnecessarily #96979)

"Commitments":

BinaryHeap::<T, A>::clone_from
VecDeque::<T, A>::clone_from
String::clone_from
HashSet::<T, S>::clone_from
OsString::clone_from
PathBuf::clone_from

@rfcbot fcp merge

rfcbot · 2024-03-31T19:03:16Z

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.

rfcbot · 2024-04-01T16:55:59Z

🔔 This is now entering its final comment period, as per the review above. 🔔

workingjubilee

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.

riking · 2024-04-05T16:54:19Z

library/core/src/cell.rs

@@ -1276,8 1276,8 @@ impl<T: Clone> Clone for RefCell<T> {
 /// Panics if `other` is currently mutably borrowed.


nit: parameter rename missed in the comment.

coolreader18 · 2024-04-11T06:28:46Z

I'm unsure of how the FCP interacts with reviews - should I address the nits/review comments while the FCP is active?

rfcbot · 2024-04-11T16:58:44Z

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.

dtolnay · 2024-04-11T18:36:09Z

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").

dtolnay

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).

dtolnay · 2024-04-17T06:15:54Z

@bors r

dtolnay · 2024-04-17T06:17:29Z

@bors ping

dtolnay · 2024-04-17T06:19:52Z

@bors ping

bors · 2024-04-17T06:19:54Z

😪 I'm awake I'm awake

dtolnay · 2024-04-17T06:20:03Z

@bors r

bors · 2024-04-17T06:20:05Z

📌 Commit 87db7c3 has been approved by dtolnay

It is now in the queue for this repository.

…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.

Document overrides of clone_from()

c0e913f

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".

rustbot assigned joboet Mar 8, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 8, 2024

coolreader18 changed the title ~~Document overrides of clone_from()~~ Document overrides of clone_from() in core/std Mar 8, 2024

rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 9, 2024

rustbot assigned dtolnay and unassigned joboet Mar 9, 2024

dtolnay requested changes Mar 31, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 31, 2024

dtolnay removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Mar 31, 2024

workingjubilee reviewed Apr 1, 2024

View reviewed changes

riking reviewed Apr 5, 2024

View reviewed changes

Address comments

87db7c3

dtolnay approved these changes Apr 17, 2024

View reviewed changes

dtolnay closed this Apr 17, 2024

dtolnay reopened this Apr 17, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 17, 2024

matthiaskrgr mentioned this pull request Apr 17, 2024

Rollup of 9 pull requests #124084

Merged

bors merged commit 21deaed into rust-lang:master Apr 17, 2024
24 checks passed

rustbot added this to the 1.79.0 milestone Apr 17, 2024

apiraino removed the to-announce Announce this issue on triage meeting label Apr 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document overrides of `clone_from()` in core/std #122201

Document overrides of `clone_from()` in core/std #122201

coolreader18 commented Mar 8, 2024 •

edited

Loading

rustbot commented Mar 8, 2024

joboet commented Mar 9, 2024

dtolnay left a comment

dtolnay Mar 31, 2024

dtolnay commented Mar 31, 2024

rfcbot commented Mar 31, 2024 •

edited by m-ou-se

Loading

rfcbot commented Apr 1, 2024

workingjubilee left a comment

riking Apr 5, 2024

coolreader18 commented Apr 11, 2024

rfcbot commented Apr 11, 2024

dtolnay commented Apr 11, 2024

dtolnay left a comment

dtolnay commented Apr 17, 2024

dtolnay commented Apr 17, 2024

dtolnay commented Apr 17, 2024

bors commented Apr 17, 2024

dtolnay commented Apr 17, 2024

bors commented Apr 17, 2024

		@@ -1276,8 1276,8 @@ impl<T: Clone> Clone for RefCell<T> {
		/// Panics if `other` is currently mutably borrowed.

Document overrides of clone_from() in core/std #122201

Document overrides of clone_from() in core/std #122201

Conversation

coolreader18 commented Mar 8, 2024 • edited Loading

rustbot commented Mar 8, 2024

joboet commented Mar 9, 2024

dtolnay left a comment

Choose a reason for hiding this comment

dtolnay Mar 31, 2024

Choose a reason for hiding this comment

dtolnay commented Mar 31, 2024

rfcbot commented Mar 31, 2024 • edited by m-ou-se Loading

rfcbot commented Apr 1, 2024

workingjubilee left a comment

Choose a reason for hiding this comment

riking Apr 5, 2024

Choose a reason for hiding this comment

coolreader18 commented Apr 11, 2024

rfcbot commented Apr 11, 2024

dtolnay commented Apr 11, 2024

dtolnay left a comment

Choose a reason for hiding this comment

dtolnay commented Apr 17, 2024

dtolnay commented Apr 17, 2024

dtolnay commented Apr 17, 2024

bors commented Apr 17, 2024

dtolnay commented Apr 17, 2024

bors commented Apr 17, 2024

Document overrides of `clone_from()` in core/std #122201

Document overrides of `clone_from()` in core/std #122201

coolreader18 commented Mar 8, 2024 •

edited

Loading

rfcbot commented Mar 31, 2024 •

edited by m-ou-se

Loading