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

Use mem::take instead of mem::replace when applicable #4881

Merged
merged 8 commits into from
Jan 4, 2020

Conversation

krishna-veerareddy
Copy link
Contributor

@krishna-veerareddy krishna-veerareddy commented Dec 4, 2019

std::mem::take can be used to replace a value of type T with T::default() instead of std::mem::replace.

Fixes issue #4871

changelog: Added lint for [mem_replace_with_default]

@krishna-veerareddy krishna-veerareddy changed the title Use mem::take instead of mem::replace when applicable [WIP] Use mem::take instead of mem::replace when applicable Dec 4, 2019
@krishna-veerareddy krishna-veerareddy changed the title [WIP] Use mem::take instead of mem::replace when applicable Use mem::take instead of mem::replace when applicable Dec 4, 2019
@flip1995 flip1995 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties A-lint Area: New lints labels Dec 5, 2019
@krishna-veerareddy krishna-veerareddy force-pushed the issue-4871-use-mem-take branch 2 times, most recently from 0f6b46c to 38bb08d Compare December 8, 2019 03:10
tests/ui/mem_replace.rs Show resolved Hide resolved
tests/ui/mem_replace.rs Show resolved Hide resolved
@krishna-veerareddy
Copy link
Contributor Author

@lzutao Hey I added checks so that it doesn't lint within macros. Also added some macro test cases to prevent regressions in the future.

@bors
Copy link
Contributor

bors commented Dec 21, 2019

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

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Linting in internal macros would be good though. On how to test this see my comment #4881 (comment)

clippy_lints/src/mem_replace.rs Outdated Show resolved Hide resolved
@flip1995
Copy link
Member

And sorry for the late reply, I was on vacation.

clippy_lints/src/mem_replace.rs Outdated Show resolved Hide resolved
clippy_lints/src/mem_replace.rs Outdated Show resolved Hide resolved
clippy_lints/src/mem_replace.rs Outdated Show resolved Hide resolved
clippy_lints/src/mem_replace.rs Outdated Show resolved Hide resolved
clippy_lints/src/mem_replace.rs Outdated Show resolved Hide resolved
@tesuji
Copy link
Contributor

tesuji commented Dec 22, 2019

If you change

-            if func_args.len() == 2;
+            if let [dest, src] = &**func_args;

@krishna-veerareddy

This comment has been minimized.

@krishna-veerareddy krishna-veerareddy force-pushed the issue-4871-use-mem-take branch 2 times, most recently from 8d1f0fc to 5d8366f Compare December 24, 2019 16:01
@flip1995
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Dec 28, 2019

📌 Commit aca6815 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Dec 28, 2019

⌛ Testing commit aca6815 with merge 31eb751...

bors added a commit that referenced this pull request Dec 28, 2019
…lip1995

Use `mem::take` instead of `mem::replace` when applicable

`std::mem::take` can be used to replace a value of type `T` with `T::default()` instead of `std::mem::replace`.

Fixes issue #4871

changelog: Added lint for [`mem_replace_with_default`]
@bors
Copy link
Contributor

bors commented Dec 28, 2019

💔 Test failed - checks-travis

@flip1995
Copy link
Member

Expr -> Expr<'_>

@krishna-veerareddy
Copy link
Contributor Author

@bors retry

@bors
Copy link
Contributor

bors commented Dec 29, 2019

@krishna-veerareddy: 🔑 Insufficient privileges: not in try users

@krishna-veerareddy
Copy link
Contributor Author

@flip1995 Hey I added the rustup fixes. Please review.

@flip1995
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Dec 30, 2019

📌 Commit 45e7b87 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Dec 30, 2019

⌛ Testing commit 45e7b87 with merge 0950766...

bors added a commit that referenced this pull request Dec 30, 2019
…lip1995

Use `mem::take` instead of `mem::replace` when applicable

`std::mem::take` can be used to replace a value of type `T` with `T::default()` instead of `std::mem::replace`.

Fixes issue #4871

changelog: Added lint for [`mem_replace_with_default`]
@bors
Copy link
Contributor

bors commented Dec 30, 2019

💔 Test failed - status-appveyor

@krishna-veerareddy
Copy link
Contributor Author

krishna-veerareddy commented Dec 30, 2019

@flip1995 @phansch Hey there was another rustup fix required so rebased again.

@krishna-veerareddy
Copy link
Contributor Author

@phansch @flip1995 Rebased after rustup. Please review when you have some time. Thanks!

@phansch
Copy link
Member

phansch commented Jan 4, 2020

@bors r=flip1995

@bors
Copy link
Contributor

bors commented Jan 4, 2020

📌 Commit 42e4595 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Jan 4, 2020

⌛ Testing commit 42e4595 with merge fa9b85d...

bors added a commit that referenced this pull request Jan 4, 2020
…lip1995

Use `mem::take` instead of `mem::replace` when applicable

`std::mem::take` can be used to replace a value of type `T` with `T::default()` instead of `std::mem::replace`.

Fixes issue #4871

changelog: Added lint for [`mem_replace_with_default`]
@bors
Copy link
Contributor

bors commented Jan 4, 2020

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

@bors bors merged commit 42e4595 into rust-lang:master Jan 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants