rust-unofficial / patterns

A catalogue of Rust design patterns, anti-patterns and idioms
https://rust-unofficial.github.io/patterns/
Mozilla Public License 2.0
8.02k stars 367 forks source link

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

Closed ve-nt closed 3 years ago

ve-nt commented 3 years ago

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 commented 3 years ago

96 can be closed with this

ve-nt commented 3 years ago

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

simonsan commented 3 years ago

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

No problem. ;-)

MarcoIeni commented 3 years ago

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

simonsan commented 3 years ago

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

https://github.com/rust-unofficial/patterns/pull/200#discussion_r556477705

Talked about it here, still neded to be discussed though

fade2black commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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?