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

Talk about mem::take instead of mem::replace #200

Merged
merged 8 commits into from
Jan 13, 2021
Merged

Talk about mem::take instead of mem::replace #200

merged 8 commits into from
Jan 13, 2021

Conversation

ve-nt
Copy link
Contributor

@ve-nt ve-nt commented Jan 13, 2021

The motive behind this PR is that mem::take is often neater and more elegant than mem::replace, especially in the example listed in the original mem::replace article.

This PR changes the mem::replace article to instead focus on mem::take. The article does mention mem::replace as an alternative if you don't want the default value to be the src, or if your type doesn't implement Default.

This also fixes a typo in the source code block that prevented the block from being compiled and executed, and then fixes a compile error in the block itself.

EDIT of Maintainer: Closes #96

@ve-nt ve-nt changed the title Mem replace take Talk about mem::take instead of mem::replace Jan 13, 2021
@ve-nt
Copy link
Contributor Author

ve-nt commented Jan 13, 2021

#96 can be closed with this

@simonsan simonsan added the C-enhancement Category: Enhancements to content label Jan 13, 2021
idioms/mem-take.md Outdated Show resolved Hide resolved
SUMMARY.md Outdated Show resolved Hide resolved
idioms/mem-take.md Outdated Show resolved Hide resolved
idioms/mem-take.md Outdated Show resolved Hide resolved
@ve-nt ve-nt requested a review from simonsan January 13, 2021 13:43
@ve-nt
Copy link
Contributor Author

ve-nt commented Jan 13, 2021

Sorry, ignore the ping to re-review. I though that was approved before my changes.

@simonsan
Copy link
Collaborator

Sorry, ignore the ping to re-review. I though that was approved before my changes.

No problem. ;-)

@MarcoIeni
Copy link
Collaborator

Should we rename the file to mem-take.md?
Other than this it looks good to me :)

@simonsan
Copy link
Collaborator

simonsan commented Jan 13, 2021

Should we rename the file to mem-take.md?
Other than this it looks good to me :)

#200 (comment)

Talked about it here, still neded to be discussed though

@MarcoIeni MarcoIeni merged commit f78e3e3 into rust-unofficial:master Jan 13, 2021
@ve-nt ve-nt deleted the mem_replace_take branch January 13, 2021 23:59
@fade2black
Copy link
Contributor

fade2black commented Jan 22, 2021

Regarding MyEnum example. In comments it says that

this takes out our name and put in an empty String instead
(note that empty strings don't allocate).
Then, construct the new enum variant (which will
be assigned to *e, because it is the result of the if let expression).

I find (note that empty strings don't allocate) part a bit misleading. As if, if empty string allocated then we couldn't write like this. Wouldn't the allocated mem be dropped even if it allocated? Just after *e = if let... assignment.
I replaced MyEnum::B { name: mem::take(name) } with MyEnum::B { name: mem::replace(name, "some-str".to_string()) }. It works fine. But I agree writing MyEnum::B { name: mem::replace(name, "some-str".to_string()) } is a nonsense.

@pickfire
Copy link
Contributor

I find (note that empty strings don't allocate) part a bit misleading. As if, if empty string allocated then we couldn't write like this. Wouldn't the allocated mem be dropped even if it allocated? Just after *e = if let... assignment.

From the docs.

Creates a new empty String.

Given that the String is empty, this will not allocate any initial buffer. While that means that this initial operation is very inexpensive, it may cause excessive allocation later when you add data. If you have an idea of how much data the String will hold, consider the with_capacity method to prevent excessive re-allocation.

Empty String does not allocate.

I don't understand why is that a nonsense.

@fade2black
Copy link
Contributor

fade2black commented Jan 22, 2021

@pickfire
When I read (note that empty strings don't allocate) it seemed to me a sort of warning, and first thing came to my mind "why note? won't it work if it allocates?". I tried replace instead of take. It worked.

I don't understand why is that a nonsense.

I mean writing MyEnum::B { name: mem::replace(name, "some-str".to_string()) } instead of MyEnum::B { name: mem::take(name) } is meaningless. The right thing is to take and leave an empty string which does not allocate. Why would we replace with another string if it is going to be dropped after all?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Enhancements to content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mem-take instead of mem-replace
5 participants