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

Add std::mem::take as suggested in #61129 #61130

Merged
merged 1 commit into from Jun 7, 2019

Conversation

Projects
None yet
@jonhoo
Copy link
Contributor

commented May 24, 2019

This PR implements #61129 by adding std::mem::take.

The added function is equivalent to:

std::mem::replace(dest, Default::default())

This particular pattern is fairly common, especially when implementing Future::poll, where you often need to yield an owned value in Async::Ready. This change allows you to write

return Async::Ready(std::mem::take(self.result));

instead of

return Async::Ready(std::mem::replace(self.result, Vec::new()));

EDIT: Changed name from take to swap_default.
EDIT: Changed name back to take.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

r? @sfackler

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

@cramertj

This comment has been minimized.

Copy link
Member

commented May 24, 2019

naming nit: mem::replace_with_default?

@jonhoo

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

@cramertj I sort of intentionally wanted it to be relatively short and concise. I see the point about wanting the name to indicate what gets left in its place though. The signature would likely tell you very quickly, but we may want it reflected in the name as well. Maybe mem::swap_new?

@cramertj

This comment has been minimized.

Copy link
Member

commented May 24, 2019

swap_default would work for me.

@jonhoo jonhoo force-pushed the jonhoo:mem-take branch from 3414d2f to ade8c8f May 24, 2019

@jonhoo jonhoo changed the title Add std::mem::take as suggested in #61129 Add std::mem::swap_default as suggested in #61129 May 24, 2019

@czipperz

This comment has been minimized.

Copy link
Contributor

commented May 25, 2019

For prior art: naming as take has precedence with Option::take and Cell::take. This is implementing a similar function to these stable methods.

@sfackler sfackler added the T-libs label May 26, 2019

@sfackler

This comment has been minimized.

Copy link
Member

commented May 26, 2019

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

commented May 26, 2019

Team member @sfackler 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.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented May 26, 2019

I’d prefer this be named replace_with_default, or take.

This is closer to a replace to a swap, since there is no second &mut T passed. It even calls replace. And replace_default sounds backwards, it’s not the default who is being replaced, but the &mut T argument. So replace_with_default. Accurate, but kinda lengthy.

take is less obvious with the name alone, but hopefully the bound in the full signature is suggestive enough: fn take<T>(&mut T) -> T where T: Default. And as @czipperz noted there’s precedent in Cell and Option.

@jonhoo

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

I personally prefer take (as in the original suggestion) when compared to replace_with_default, as the latter is quite verbose. I could go either way though — happy to update according to consensus.

@jonhoo jonhoo force-pushed the jonhoo:mem-take branch from ade8c8f to 263b176 May 26, 2019

@jonhoo jonhoo changed the title Add std::mem::swap_default as suggested in #61129 Add std::mem::take as suggested in #61129 May 26, 2019

@timvermeulen

This comment has been minimized.

Copy link
Contributor

commented May 26, 2019

Is there any precedence for functions in the standard library that are related to Default but don't have default in the name?

@dtolnay

This comment has been minimized.

Copy link
Member

commented May 26, 2019

Yes -- #61130 (comment) mentions Cell::take.

@timvermeulen

This comment has been minimized.

Copy link
Contributor

commented May 26, 2019

Ah, I saw that, but wasn't aware of how it works. I have no objections then.

@czipperz

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

Make sure to update the pr/issue to reflect the take name. The PR's example is messed up.

@rfcbot

This comment has been minimized.

Copy link

commented May 28, 2019

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

@fhartwig

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

take is less obvious with the name alone, but hopefully the bound in the full signature is suggestive enough: fn take<T>(&mut T) -> T where T: Default. And as @czipperz noted there’s precedent in Cell and Option.

In Option's case the replacement is arguably obvious, though. And in my opinion having a suggestive type signature doesn't really help, considering you'll only see that when looking up the documentation.

@bors

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

☔️ The latest upstream changes (presumably #61317) made this pull request unmergeable. Please resolve the merge conflicts.

Add std::mem::take as suggested in #61129
The name `swap_default` was suggested but rejected. @SimonSapin observed
that this operation isn't really a `swap` in the same sense as
`mem::swap`; it is a `replace`. Since `replace_default` is a bit
misleading, the "correct" name would be `replace_with_default`, which is
quite verbose.

@czipperz observed that we have precedence for using `take` to refer to
methods that replace with `Default` in `Cell::take` and `Option::take`,
so this reverts commit 99c0059 to
return to the original `take` method name.

The name `replace_with_default` was suggested, but was deemed too
verbose, especially given that we use `take` for methods that replace
with `Default` elsewhere.

@jonhoo jonhoo force-pushed the jonhoo:mem-take branch from 263b176 to 5a01b54 May 29, 2019

@scottmcm

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

Given that it's a function, I like the take name since I'd end up always calling it as mem::take, which is distinct enough for me to remember what's going on.

@rfcbot

This comment has been minimized.

Copy link

commented Jun 7, 2019

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.

The RFC will be merged soon.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

📌 Commit 5a01b54 has been approved by SimonSapin

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

⌛️ Testing commit 5a01b54 with merge f5e8c96...

bors added a commit that referenced this pull request Jun 7, 2019

Auto merge of #61130 - jonhoo:mem-take, r=SimonSapin
Add std::mem::take as suggested in #61129

This PR implements #61129 by adding `std::mem::take`.

The added function is equivalent to:
```rust
std::mem::replace(dest, Default::default())
```

This particular pattern is fairly common, especially when implementing `Future::poll`, where you often need to yield an owned value in `Async::Ready`. This change allows you to write
```rust
return Async::Ready(std::mem::take(self.result));
```
instead of
```rust
return Async::Ready(std::mem::replace(self.result, Vec::new()));
```

EDIT: Changed name from `take` to `swap_default`.
EDIT: Changed name back to `take`.

Centril added a commit to Centril/rust that referenced this pull request Jun 7, 2019

Rollup merge of rust-lang#61130 - jonhoo:mem-take, r=SimonSapin
Add std::mem::take as suggested in rust-lang#61129

This PR implements rust-lang#61129 by adding `std::mem::take`.

The added function is equivalent to:
```rust
std::mem::replace(dest, Default::default())
```

This particular pattern is fairly common, especially when implementing `Future::poll`, where you often need to yield an owned value in `Async::Ready`. This change allows you to write
```rust
return Async::Ready(std::mem::take(self.result));
```
instead of
```rust
return Async::Ready(std::mem::replace(self.result, Vec::new()));
```

EDIT: Changed name from `take` to `swap_default`.
EDIT: Changed name back to `take`.

@Centril Centril referenced this pull request Jun 7, 2019

Closed

Rollup of 7 pull requests #61630

@Centril

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

@bors retry r0lled up

bors added a commit that referenced this pull request Jun 7, 2019

Auto merge of #61630 - Centril:rollup-2u0y4h9, r=Centril
Rollup of 7 pull requests

Successful merges:

 - #61130 (Add std::mem::take as suggested in #61129)
 - #61597 (Bump dirs, rand and redox_users)
 - #61615 (syntax: Treat error literals in more principled way)
 - #61616 (parser: Remove `Deref` impl from `Parser`)
 - #61621 (Clarify when we run steps with ONLY_HOSTS)
 - #61625 (Rename remaining "Eval" to "Interp")
 - #61627 (Add regression test for #61452.)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

⌛️ Testing commit 5a01b54 with merge d132f54...

bors added a commit that referenced this pull request Jun 7, 2019

Auto merge of #61130 - jonhoo:mem-take, r=SimonSapin
Add std::mem::take as suggested in #61129

This PR implements #61129 by adding `std::mem::take`.

The added function is equivalent to:
```rust
std::mem::replace(dest, Default::default())
```

This particular pattern is fairly common, especially when implementing `Future::poll`, where you often need to yield an owned value in `Async::Ready`. This change allows you to write
```rust
return Async::Ready(std::mem::take(self.result));
```
instead of
```rust
return Async::Ready(std::mem::replace(self.result, Vec::new()));
```

EDIT: Changed name from `take` to `swap_default`.
EDIT: Changed name back to `take`.
@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: SimonSapin
Pushing d132f54 to master...

@bors bors added the merged-by-bors label Jun 7, 2019

@bors bors merged commit 5a01b54 into rust-lang:master Jun 7, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.