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.

Sign up for GitHub

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

Jump to bottom

Lint must_use on mem::replace #71256

Merged
merged 3 commits into from
Apr 23, 2020
Merged

Lint must_use on mem::replace #71256

merged 3 commits into from
Apr 23, 2020

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Apr 17, 2020

This adds a hint on mem::replace, "if you don't need the old value,
you can just assign the new value directly". This is in similar spirit
to the must_use on ManuallyDrop::take.

@cuviper
Lint must_use on mem::replace
7c4ca59 
This adds a hint on `mem::replace`, "if you don't need the old value,
you can just assign the new value directly". This is in similar spirit
to the `must_use` on `ManuallyDrop::take`.
@rust-highfive
Copy link
Collaborator

r? @kennytm

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 17, 2020
@rust-highfive

This comment has been minimized.

Sign in to view
@cuviper
Copy link
Member Author

cuviper commented Apr 17, 2020

Huh, I didn't expect that this would already affect code in the compiler itself. I'll look into it.

@cuviper
Copy link
Member Author

cuviper commented Apr 17, 2020

OK, github-actions CI/PR passed now.

@kennytm kennytm requested a review from estebank April 19, 2020 15:52
@estebank
Copy link
Contributor

I'm ok with the code changes. Couldn't find whether the lint is only warn-by-default. If it is, then r=me, after the nitpick about the test.

@cuviper
Copy link
Member Author

cuviper commented Apr 20, 2020

I'm ok with the code changes. Couldn't find whether the lint is only warn-by-default. If it is, then r=me, after the nitpick about the test.

Yes, it is a warning by default -- the way that I check is with -Whelp:

$ rustc -Whelp | grep must_use
        unused-must-use  warn     unused result of a type flagged as `#[must_use]`

I addressed the test nitpick too, so...

@bors r=estebank

@bors
Copy link
Contributor

bors commented Apr 20, 2020

📌 Commit 7fca9f8 has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 20, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 20, 2020
@Dylan-DPC
Rollup merge of rust-lang#71256 - cuviper:must_use_replace, r=estebank
c3e4026 
Lint must_use on mem::replace

This adds a hint on `mem::replace`, "if you don't need the old value,
you can just assign the new value directly". This is in similar spirit
to the `must_use` on `ManuallyDrop::take`.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Apr 20, 2020
Closed
@Dylan-DPC-zz
Copy link

fails since cargo doesn't use the result somewhere, (fixing it)

@Dylan-DPC-zz
Copy link

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 20, 2020
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Apr 20, 2020
Merged
@kennytm kennytm added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 20, 2020
bors added a commit to rust-lang/cargo that referenced this pull request Apr 20, 2020
@bors
Auto merge of #8138 - Dylan-DPC:fix/transmute_unused, r=alexcrichton
6a41339 
fix mem replace unused

`mem::replace` will be linted as must_use, so modifying this.

This is currently blocking rust-lang/rust#71256
@cuviper
Copy link
Member Author

cuviper commented Apr 20, 2020

I see, cargo already had a warning in the normal build, but #![cfg_attr(test, deny(warnings))] stopped it in full CI. Thanks for going ahead with that fix!

@Dylan-DPC-zz
Copy link

the blocking pr is now merged

@bors r=estebank

@bors
Copy link
Contributor

bors commented Apr 20, 2020

📌 Commit 7fca9f8 has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Apr 20, 2020
@cuviper
Copy link
Member Author

cuviper commented Apr 20, 2020

Do I need to update the cargo submodule too? Or are you handling that elsewhere?

@Dylan-DPC-zz
Copy link

Uh right we probably have to update cargo first

@Dylan-DPC-zz
Copy link

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 20, 2020
@Dylan-DPC-zz Dylan-DPC-zz added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 20, 2020
@ehuss
Copy link
Contributor

ehuss commented Apr 20, 2020

I'll post a cargo update tomorrowish after the beta branch. There are several big changes that I don't feel comfortable sliding into beta at the last moment.

@ehuss ehuss mentioned this pull request Apr 21, 2020
Merged
@cuviper cuviper mentioned this pull request Apr 22, 2020
Closed
@cuviper
Copy link
Member Author

cuviper commented Apr 22, 2020

Cargo was updated in #71402, so let's give this another shot.

@bors r=estebank

@bors
Copy link
Contributor

bors commented Apr 22, 2020

📌 Commit 7fca9f8 has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Apr 22, 2020
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Apr 22, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 23, 2020
@bors
Auto merge of rust-lang#71445 - Dylan-DPC:rollup-31givp1, r=Dylan-DPC
fc145e1 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#71256 (Lint must_use on mem::replace)
 - rust-lang#71350 (Error code explanation extra check)
 - rust-lang#71369 (allow wasm32 compilation of librustc_data_structures/profiling.rs)
 - rust-lang#71400 (proc_macro::is_available())
 - rust-lang#71440 (Implement `Copy` for `AllocErr`)

Failed merges:

r? @ghost
@bors bors merged commit 10e47f5 into rust-lang:master Apr 23, 2020
nicokoch referenced this pull request in gridbugs/rodio Apr 23, 2020
@gridbugs
Add comments explaining Drop impl for FrameData
99c0c36
@nicokoch nicokoch mentioned this pull request Apr 23, 2020
Merged
@taiki-e taiki-e mentioned this pull request May 12, 2020
Closed
@cuviper cuviper deleted the must_use_replace branch May 30, 2020 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estebank estebank Awaiting requested review from estebank

Assignees

@kennytm kennytm

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@cuviper @rust-highfive @estebank @bors @Dylan-DPC-zz @ehuss @kennytm