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 dead or useless code from Session #83596

Merged
merged 5 commits into from
May 9, 2021
Merged

Remove dead or useless code from Session #83596

merged 5 commits into from
May 9, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Mar 28, 2021 •
edited
Loading

This is a more principled follow-up to #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

@jyn514 jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 28, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 28, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jyn514 jyn514 force-pushed the session-dead-code branch from c4e33e2 to f9b26a6 Compare March 28, 2021 03:17
bjorn3
bjorn3 reviewed Mar 28, 2021
View reviewed changes
compiler/rustc_session/src/session.rs Outdated
fn deref(&self) -> &Self::Target {
self.diagnostic()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This is not very clean IMO and prevents deref to any other type in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea... this always was the "obvious" way to eliminate the duplication and we didn't do it on purpose.

we could add a diag() method on TyCtxt and Session and "just" use it everywhere. This would change all tcx.sess.span... to tcx.diag().span..., so it isn't really more verbose.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I wonder if that should go through MCP - it would be a lot of work and I'd hate to throw it away because someone didn't like the new API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, do an MCP or bring it up with @estebank directly before opening an MCP

@bors

This comment has been minimized.

Sign in to view
@jyn514 jyn514 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 30, 2021
@crlf0710 crlf0710 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 17, 2021
@jyn514 jyn514 force-pushed the session-dead-code branch from f9b26a6 to 0510ca7 Compare April 22, 2021 16:24
@bors

This comment has been minimized.

Sign in to view
jyn514 added 5 commits May 8, 2021 23:11
@jyn514
Rename Parser::span_fatal_err -> Parser::span_err
955fdae 
The name was misleading, it wasn't actually a fatal error.
@jyn514
Remove some unnecessary uses of struct_span_fatal
e49f447 
All of them immediately called `emit()` then `raise()`, so they could
just call `span_fatal` directly.
@jyn514
Make Diagnostic::span_fatal unconditionally raise an error
96509b4 
It had no callers which didn't immediately call `raise()`, and this
unifies the behavior with `Session`.
@jyn514
Note why Handler::fatal is different from Sesssion::fatal
ebbc949
@jyn514
Remove unused opt_span_warn function
f25aa57
@jyn514 jyn514 force-pushed the session-dead-code branch from 0510ca7 to f25aa57 Compare May 9, 2021 03:16
@jyn514
Copy link
Member Author

jyn514 commented May 9, 2021 •
edited
Loading

I don't have time to follow up with this - I think the other commits are good, I've dropped 245811f. If there are review comments I will likely not have time to address them unfortunately, at least for the next month or so.

@oli-obk
Copy link
Contributor

oli-obk commented May 9, 2021

@bors r+

@bors
Copy link
Contributor

bors commented May 9, 2021

📌 Commit f25aa57 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 9, 2021
@bors
Copy link
Contributor

bors commented May 9, 2021

⌛ Testing commit f25aa57 with merge bba8710...

@bors
Copy link
Contributor

bors commented May 9, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 9, 2021
@bors bors merged commit bba8710 into rust-lang:master May 9, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 9, 2021
@jyn514 jyn514 deleted the session-dead-code branch May 9, 2021 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

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

Assignees

@oli-obk oli-obk

Labels
A-diagnostics Area: Messages for errors, warnings, and lints 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.54.0
Development

Successfully merging this pull request may close these issues.
8 participants
@jyn514 @rust-log-analyzer @bors @oli-obk @bjorn3 @crlf0710 @rust-highfive @rustbot