-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Remove an unnecessary unwrap in rustc_codegen_gcc
#149449
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
Remove an unnecessary unwrap in rustc_codegen_gcc
#149449
Conversation
|
Some changes occurred in compiler/rustc_codegen_gcc |
|
Yeah, it should be fine. @bors r+ |
Rollup merge of #149449 - AudaciousAxiom:refactor/rustc-codegen-gcc-remove-unnecessary-unwrap, r=workingjubilee Remove an unnecessary `unwrap` in `rustc_codegen_gcc` This should hopefully unblock #149425 (I couldn't find an in-flight PR that was already doing this). I've tested locally with the `master` version of Clippy that `rustc_codegen_gcc` passes the lints (the syncing PR could still fail for other reasons however). I understand that `rustc_codegen_gcc` is normally developed [outside of this repo](https://github.com/rust-lang/rustc_codegen_gcc) but my understanding is that that repo is two-way synced regularly and hopefully it is acceptable to do this tiny change here to unblock the Clippy syncing PR (is there an established process for how to unblock these syncing PRs?). Of course feel free to close if this isn't the expected process.
| if let Some(location) = bx.location { | ||
| #[cfg(feature = "master")] | ||
| rvalue.set_location(bx.location.unwrap()); | ||
| rvalue.set_location(location); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably going to cause some clippy failures on our side, though.
Better would have been:
| if let Some(location) = bx.location { | |
| #[cfg(feature = "master")] | |
| rvalue.set_location(bx.location.unwrap()); | |
| rvalue.set_location(location); | |
| } | |
| #[cfg(feature = "master")] | |
| if let Some(location) = bx.location { | |
| rvalue.set_location(location); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. It seems unlikely that it would do so, @antoyo, given it fixes a clippy lint? But I will try to pay closer attention to the cfg on any PRs in this area.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it might seem weird as an "empty" conditional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the feature master is disabled, the variable location is unused. That might even be a compiler warning instead of a clippy warning.
This should hopefully unblock #149425 (I couldn't find an in-flight PR that was already doing this).
I've tested locally with the
masterversion of Clippy thatrustc_codegen_gccpasses the lints (the syncing PR could still fail for other reasons however).I understand that
rustc_codegen_gccis normally developed outside of this repo but my understanding is that that repo is two-way synced regularly and hopefully it is acceptable to do this tiny change here to unblock the Clippy syncing PR (is there an established process for how to unblock these syncing PRs?). Of course feel free to close if this isn't the expected process.r? @matthiaskrgr