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

Tracking issue for std::mem::take #61129

Closed
jonhoo opened this issue May 24, 2019 · 13 comments
Closed

Tracking issue for std::mem::take #61129

jonhoo opened this issue May 24, 2019 · 13 comments
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@jonhoo
Copy link
Contributor

jonhoo commented May 24, 2019

This tracks the stabilization of this function added by PR #61130 in std::mem:

pub fn take<T: Default>(dest: &mut T) -> T {
    replace(dest, T::default())
}

Original feature request:


I decently often find myself in a position where I have a HashMap, Vec, or some other non-Copy type I want to "take" from its current location, but cannot because I only have &mut self. A particularly common place for this to happen is when finally returning Async::Ready from Future::poll. Usually in these cases, I end up writing:

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

This works pretty well, since Vec::new does not allocate (as is the case for most collections).

This pattern is common enough that it'd be nice if there was a more standard way to do it. In particular, something like:

mod std::mem {
    fn take<T: Default>(t: &mut T) -> T {
        std::mem::replace(t, Default::default())
    }
}

A variant of this was suggested back in #33564, though that modified Default instead, which seems like overkill.

If this is something others agree would be useful, I'd be happy to submit a PR.

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

@Centril Centril added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-enhancement Category: An issue proposing an enhancement or a PR with one. C-feature-request Category: A feature request, i.e: not implemented / a PR. labels May 24, 2019
@Centril
Copy link
Contributor

Centril commented May 24, 2019

Seems like a reasonable addition to me 👍

@abonander
Copy link
Contributor

I've written this helper in projects before as replace_default() which is clearer but is more verbose.

@jonhoo
Copy link
Contributor Author

jonhoo commented May 24, 2019

Implemented in #61130

@petrochenkov
Copy link
Contributor

mem::replace

mem::swap also works.
That's what C++ did before moves were introduced, because "taking" the value and leaving something cheap but valid instead is exactly that.

"Something cheap but valid" is not necessarily Default, so in C++ it's leaved to decide to the type itself and Rust analogue would be something like

trait Steal {
    fn steal(&mut self) -> Self;
}

Default implementations for T: Copy and T: Default types are possible, except they will probably conflict without specialization.

@jonhoo
Copy link
Contributor Author

jonhoo commented May 24, 2019

mem::swap also works.

@petrochenkov not sure what you mean? I don't think you can do this with mem::swap without separately producing the value in the caller?

As for adding a trait, we could totally do that, though Default seemed like a decent candidate for filler value to me. Can you think of types where you would not want Default, but there is some other acceptable filler value?

@petrochenkov
Copy link
Contributor

@jonhoo

Can you think of types where you would not want Default, but there is some other acceptable filler value?

Trivial example - for any Copy type it's faster to just memcpy it than e.g. memcpy + reset the origin to default value.
Can't give a non-copying example right away, need to think.

@jonhoo jonhoo changed the title std::mem::take when T implements Default std::mem::swap_default when T implements Default May 24, 2019
@czipperz
Copy link
Contributor

for any Copy type it's faster to just memcpy it than e.g. memcpy + reset the origin to default value.

Hopefully that assignment will be eliminated by the optimizer

@jonhoo jonhoo changed the title std::mem::swap_default when T implements Default std::mem::take when T implements Default May 27, 2019
jonhoo added a commit to jonhoo/rust that referenced this issue May 29, 2019
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 99c00591c29b472c8a87c4a9342d0e0c508647a3 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.
@SimonSapin SimonSapin added B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. and removed C-enhancement Category: An issue proposing an enhancement or a PR with one. C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Jun 7, 2019
@SimonSapin SimonSapin changed the title std::mem::take when T implements Default Tracking issue for std::mem::take Jun 7, 2019
@SimonSapin
Copy link
Contributor

The implementation PR #61130 is landing, I’ve edited the issue description and labels to make it a tracking issue.

Centril added a commit to Centril/rust that referenced this issue Jun 7, 2019
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`.
bors added a commit that referenced this issue Jun 7, 2019
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`.
SimonSapin added a commit to SimonSapin/victor that referenced this issue Jun 10, 2019
SimonSapin added a commit to SimonSapin/victor that referenced this issue Jun 13, 2019
@czipperz
Copy link
Contributor

At some point, we should revert commit ac21b60, which doesn't currently build. (This will convert compiletest usage of replace to take).

@czipperz
Copy link
Contributor

czipperz commented Jul 2, 2019

^This is tracked in #62306. It should be done once this is stabilized.

@matklad
Copy link
Member

matklad commented Sep 23, 2019

@jonhoo would you like to send a stabilization PR? I feel like this is pretty uncontroversial and useful, to the point that I first type mem::take and than recall that it is not stable yet :-)

@jonhoo
Copy link
Contributor Author

jonhoo commented Sep 23, 2019

@matklad done in #64716

jonhoo added a commit to jonhoo/rust that referenced this issue Oct 8, 2019
bors added a commit that referenced this issue Oct 11, 2019
Stabilize mem::take (mem_take)

Tracking issue: #61129

r? @matklad
@SimonSapin
Copy link
Contributor

Stabilized in #64716.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants