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

Remove (lots of) dead code #83185

Merged
merged 6 commits into from
Mar 29, 2021
Merged

Remove (lots of) dead code #83185

merged 6 commits into from
Mar 29, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Mar 16, 2021 •
edited
Loading

Builds on

Found with https://github.com/est31/warnalyzer.
See #77739 for a similar change in the past.

Dubious changes:

  • Maybe some of the dead code in rustc_data_structures should be kept, in case someone wants to use it in the future?

TODO:

cc @est31

@jyn514 jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 16, 2021
@rust-highfive
Copy link
Collaborator

r? @oli-obk

(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 Mar 16, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jyn514 jyn514 force-pushed the remove-dead-code branch 2 times, most recently from 86e1dd2 to 08e596a Compare March 16, 2021 06:22
@jyn514
Copy link
Member Author

jyn514 commented Mar 16, 2021

@bors try @rust-timer queue

I expect this to help hello-world, but not much else.

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 16, 2021
@bors
Copy link
Contributor

bors commented Mar 16, 2021

⌛ Trying commit 08e596a84bea96a0d8c3799c36724c012bb9154b with merge cee392aca3bbc45102766189904bd3c29f498f19...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jyn514 jyn514 force-pushed the remove-dead-code branch 2 times, most recently from 3dd122c to aa09c5b Compare March 16, 2021 06:38
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@thomcc
Copy link
Member

thomcc commented Mar 16, 2021

Hmm, it seems likely that the resolution of some of our floating point bugs will require more architecture-awareness of floats, rather than less, but I guess if we aren't using it we could just resurrect the code from git history if that time comes...

@oli-obk
Copy link
Contributor

oli-obk commented Mar 16, 2021

Is anyone else using rustc_apfloat? I feel weird completely deleting x87 support.

Please don't touch rustc_apfloat without conferring with @eddyb . They manually transpiled llvm's apfloat to Rust and this is the result. We should not touch that library afaik.

@eddyb
Copy link
Member

eddyb commented Mar 16, 2021

So the whole deal with rustc_apfloat is that it's supposed to become a long-lived "faithful translation" that tracks upstream (i.e. backporting newer bug fixes), and also we try to also upstream any bug fixes we come up with.

It shouldn't even be in-tree IMO, see also #55993 - people have wanted to use it elsewhere, too.

But I haven't been able to do as much as I want with it because it's been stuck in licensing limbo for years.

I don't dare to touch it at all, really, while the licensing situation is up in the air, in the fear that it will prolong the limbo (originally I was assured there were no licensing issues and whoever told me that was wrong, so I don't want that to happen again) and I'd strongly advise anyone else against messing with it.

(Can we add a rule somewhere that makes rust-highfive ping me whenever someone touches compiler/rustc_apfloat?)

Once the licensing situation is sorted out, and we can move it out of tree, publish on crates.io, etc. I want to take this "two main branches" approach, to cleanly separate what's "tracking LLVM upstream" and what's our own patches: #55993 (comment)

eddyb
eddyb reviewed Mar 16, 2021
View reviewed changes
compiler/rustc_trait_selection/src/traits/query/type_op/implied_outlives_bounds.rs
Comment on lines -11 to -15
impl<'tcx> ImpliedOutlivesBounds<'tcx> {
pub fn new(ty: Ty<'tcx>) -> Self {
ImpliedOutlivesBounds { ty }
}
}
Copy link
Member

Choose a reason for hiding this comment

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

cc @nikomatsakis unsure what's going on here.

@jyn514
Copy link
Member Author

jyn514 commented Mar 28, 2021

I added back all the functions @oli-obk asked I keep, this should be ready for another round of review.

@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 28, 2021
@jyn514 jyn514 mentioned this pull request Mar 28, 2021
Merged
@bjorn3 bjorn3 mentioned this pull request Mar 28, 2021
Merged
@oli-obk
Copy link
Contributor

oli-obk commented Mar 29, 2021 •
edited
Loading

Except for the sync stuff, everything lgtm. I have no strong opinion, but since I have good hopes that the work on parallel compiler will continue this year, I'm worried this removes things we'll just need to add again. I don't know who to ping about it right now though

@jyn514
Copy link
Member Author

jyn514 commented Mar 29, 2021

Except for the sync stuff, everything lgtm. I have no strong opinion, but since I have good hopes that the work on parallel compiler will continue this year, I'm worried this removes things we'll just need to add again. I don't know who to ping about it right now though

It looks like SerialScope was added as part of #50235, and AtomicCell as part of #62577.

cc @Zoxc - do these changes to rustc_data_structures::sync look ok?

@jyn514
Revert changes to sync data structures
526bb10 
There isn't currently a good reviewer for these, and I don't want to
remove things that will just be added again. I plan to make a separate
PR for these changes so the rest of the cleanup can land.
@jyn514
Copy link
Member Author

jyn514 commented Mar 29, 2021

I reverted the sync changes, I'll make a separate PR for them so they don't hold up everything else.

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Mar 29, 2021

📌 Commit 526bb10 has been approved by oli-obk

@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 Mar 29, 2021
@jyn514 jyn514 mentioned this pull request Mar 29, 2021
Merged
@bors
Copy link
Contributor

bors commented Mar 29, 2021

⌛ Testing commit 526bb10 with merge 48691ea...

@bors
Copy link
Contributor

bors commented Mar 29, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 48691ea to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 29, 2021
@bors bors merged commit 48691ea into rust-lang:master Mar 29, 2021
@rustbot rustbot added this to the 1.53.0 milestone Mar 29, 2021
@jyn514 jyn514 deleted the remove-dead-code branch March 29, 2021 22:29
This was referenced Mar 29, 2021
Closed
Merged
Merged
dtolnay added a commit to dtolnay/syn that referenced this pull request Mar 30, 2021
@dtolnay
Update test suite to nightly-2021-03-30
16781e7 
rust-lang/rust#83185
@jyn514 jyn514 mentioned this pull request Mar 31, 2021
Closed
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Apr 26, 2021
Open
bors added a commit to rust-lang-ci/rust that referenced this pull request May 9, 2021
@bors
Auto merge of rust-lang#83596 - jyn514:session-dead-code, r=oli-obk
bba8710 
Remove dead or useless code from Session

This is a more principled follow-up to rust-lang#83185 (comment).

- Rename `Parser::span_fatal_err` -> `Parser::span_err`
- Remove some unnecessary uses of `struct_span_fatal`
- Make `Diagnostic::span_fatal` unconditionally raise an error
- Add `impl Deref<Target = Handler>` for Session and remove all functions that are exactly the same as their Handler counterparts
- Note why `Handler::fatal` is different from `Sesssion::fatal`
- Remove unused `opt_span_warn` function

r? `@oli-obk` or `@estebank`
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 4, 2021
@JohnTitor
Rollup merge of rust-lang#83653 - jyn514:unused-sync-code, r=wesleywiser
7564312 
Remove unused code from `rustc_data_structures::sync`

Found using https://github.com/est31/warnalyzer. Follow-up to rust-lang#83185.

r? `@Zoxc` cc `@oli-obk`
This was referenced Jun 16, 2021
Closed
Merged
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Sep 17, 2021
@JohnTitor
Rollup merge of rust-lang#86382 - JohnTitor:try-desugar, r=estebank
f4f7704 
Make diagnostics clearer for `?` operators

Re-submission of rust-lang#75029, fixes rust-lang#71309
This also revives the `content` methods removed by rust-lang#83185.
r? `@estebank`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnTitor JohnTitor JohnTitor left review comments

@bjorn3 bjorn3 bjorn3 left review comments

@richkadel richkadel richkadel left review comments

@Xanewok Xanewok Xanewok left review comments

@cjgillot cjgillot cjgillot left review comments

@oli-obk oli-obk oli-obk left review comments

@RalfJung RalfJung RalfJung left review comments

@nikomatsakis nikomatsakis nikomatsakis left review comments

@eddyb eddyb eddyb left review comments

Assignees

@oli-obk oli-obk

Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.53.0
Development

Successfully merging this pull request may close these issues.