-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 unused/unnecessary features #119968
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
cc @davidtwco, @compiler-errors, @TaKO8Ki This PR changes Stable MIR cc @oli-obk, @celinval, @spastorino, @ouz-a Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred in need_type_info.rs cc @lcnr
|
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.
What's the motivation for removing try blocks? The rest of the changes seem fine, but there's no bad reason to keep "testing" try blocks in the compiler imo.
Mostly just because it was a very easy change to make to cut down on unstable features - but yeah, I'll restore them |
Yeah, +1 to what @compiler-errors said — there isn't really a reason to avoid try blocks. And while it's possible to remove their uses, it makes the code worse imo. |
cc744dd
to
ca5d500
Compare
This comment has been minimized.
This comment has been minimized.
ca5d500
to
9a17b9f
Compare
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.
This is fine, though I guess I don't necessarily agree that we should be removing harmless libs features like .get_or_insert_with_default()
.
Should be fine tho @bors r+ |
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.
Generally, should this PR be limited to removing the attributes? Testing library features is normal in the compiler.
@bors r- Yeah, let's just revert all the libs feature changes. |
9a17b9f
to
e5a520f
Compare
@bors r+ |
☔ The latest upstream changes (presumably #120251) made this pull request unmergeable. Please resolve the merge conflicts. |
5bb0097
to
cd4a889
Compare
@bors r=compiler-errors |
@bors r- |
cd4a889
to
fd29f74
Compare
Found another conflict while fixing that one so |
☀️ Test successful - checks-actions |
1 similar comment
☀️ Test successful - checks-actions |
Finished benchmarking commit (69db514): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 663.451s -> 662.651s (-0.12%) |
The bulk of the actual code changes here is replacing try blocks with equivalent closures. I'm not entirely sure that's a good idea since it may have perf impact, happy to revert if that's the case/the change is unwanted.I also removed a lot of
recursion_limit = "256"
since everything seems to build fine without that and most don't have any comment justifying it.