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.

Sign up for GitHub

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

Jump to bottom

Implement Clone::clone_from for Option and Result #61348

Merged
merged 2 commits into from
Jun 12, 2019
Merged

Implement Clone::clone_from for Option and Result #61348

merged 2 commits into from
Jun 12, 2019

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented May 30, 2019 •
edited
Loading

See #28481

@rust-highfive
Copy link
Collaborator

r? @KodrAus

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 30, 2019
Centril
Centril previously requested changes May 30, 2019
View reviewed changes
src/libcore/option.rs Outdated Show resolved Hide resolved
@scottmcm
Copy link
Member

scottmcm commented Jun 1, 2019

These are still fully-structural impls, right? Could the #[derive] just be made to emit these?

@m-ou-se
Copy link
Member Author

m-ou-se commented Jun 1, 2019

These are still fully-structural impls, right? Could the #[derive] just be made to emit these?

There's been a few discussions on this. #27939 was closed because it would affect the compile times too much.

Also, I think it might go wrong when there are multiple fields: If some fields are succesfully clone_from'd, but another panics, the object might be left in an invalid state. But it should be possible if there's only one field.

@Centril
Copy link
Contributor

Centril commented Jun 10, 2019

@bors try

@bors
Copy link
Contributor

bors commented Jun 10, 2019

⌛ Trying commit 67fd995 with merge 1406f73...

bors added a commit that referenced this pull request Jun 10, 2019
@bors
Auto merge of #61348 - dronesforwork-forks:clone-from, r=<try>
1406f73 
Implement Clone::clone_from for Option and Result

See #28481
@bors
Copy link
Contributor

bors commented Jun 10, 2019

☀️ Try build successful - checks-travis
Build commit: 1406f73

@Centril
Copy link
Contributor

Centril commented Jun 10, 2019

@rust-timer build 1406f73

@rust-timer
Copy link
Collaborator

Success: Queued 1406f73 with parent 400b409, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 1406f73, comparison URL.

@Centril
Copy link
Contributor

Centril commented Jun 10, 2019

Looks like noise.

@KodrAus
Copy link
Contributor

KodrAus commented Jun 11, 2019

Based on the discussion in #28481 this looks good to me.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 11, 2019

📌 Commit 67fd995 has been approved by KodrAus

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 11, 2019
Centril added a commit to Centril/rust that referenced this pull request Jun 12, 2019
@Centril
Rollup merge of rust-lang#61348 - dronesforwork-forks:clone-from, r=K…
b5184e5 
…odrAus

Implement Clone::clone_from for Option and Result

See rust-lang#28481
@Centril Centril mentioned this pull request Jun 12, 2019
Merged
bors added a commit that referenced this pull request Jun 12, 2019
@bors
Auto merge of #61758 - Centril:rollup-ew2uxng, r=Centril
c4797fa 
Rollup of 9 pull requests

Successful merges:

 - #60187 (Generator optimization: Overlap locals that never have storage live at the same time)
 - #61348 (Implement Clone::clone_from for Option and Result)
 - #61568 (Use Symbol, Span in libfmt_macros)
 - #61632 (ci: Collect CPU usage statistics on Azure)
 - #61654 (use pattern matching for slices destructuring)
 - #61671 (implement nth_back for Range(Inclusive))
 - #61688 (is_fp and is_floating_point do the same thing, remove the former)
 - #61705 (Pass cflags rather than cxxflags to LLVM as CMAKE_C_FLAGS)
 - #61734 (Migrate rust-by-example to MdBook2)

Failed merges:

r? @ghost
@bors bors merged commit 67fd995 into rust-lang:master Jun 12, 2019
@m-ou-se m-ou-se deleted the clone-from branch June 12, 2019 07:51
@cuviper cuviper mentioned this pull request Jun 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Centril Centril Centril left review comments

Assignees

@KodrAus KodrAus

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@m-ou-se @rust-highfive @scottmcm @Centril @bors @rust-timer @KodrAus