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 RunCompiler::emitter. #102992

Merged
merged 1 commit into from
Oct 18, 2022
Merged

Remove RunCompiler::emitter. #102992

merged 1 commit into from
Oct 18, 2022

Conversation

nnethercote
Copy link
Contributor

It's no longer used.

r? @bjorn3

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 12, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 12, 2022
@nnethercote nnethercote mentioned this pull request Oct 12, 2022
Closed
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@nnethercote nnethercote force-pushed the rm-RunCompiler-emitter branch from 4afc313 to 0665cba Compare October 13, 2022 02:31
bjorn3
bjorn3 reviewed Oct 13, 2022
View reviewed changes
compiler/rustc_driver/src/lib.rs Outdated
/// Used by RLS.
/// Has no uses within this repository, but is used by bjorn3 for "hooking
/// rust-analyzer's VFS into rustc at some point for running rustc without
/// having to save". (See #102759.)
Copy link
Member

Choose a reason for hiding this comment

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

It isn't used for this yet, but may be in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the comment accordingly.

Other than that, are you happy with this PR?

@nnethercote
Remove RunCompiler::emitter.
641f824 
It's no longer used.
@nnethercote nnethercote force-pushed the rm-RunCompiler-emitter branch from 0665cba to 641f824 Compare October 17, 2022 21:49
@bjorn3
Copy link
Member

bjorn3 commented Oct 17, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Oct 17, 2022

📌 Commit 641f824 has been approved by bjorn3

It is now in the queue for this repository.

@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 Oct 17, 2022
@bors
Copy link
Contributor

bors commented Oct 18, 2022

⌛ Testing commit 641f824 with merge a03ca01...

@bors
Copy link
Contributor

bors commented Oct 18, 2022

☀️ Test successful - checks-actions
Approved by: bjorn3
Pushing a03ca01 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 18, 2022
@bors bors merged commit a03ca01 into rust-lang:master Oct 18, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 18, 2022
This was referenced Oct 18, 2022
Merged
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a03ca01): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This 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.

mean1 range count2
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.2% [2.2%, 2.2%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Footnotes

  1. the arithmetic mean of the percent change ↩

  2. number of relevant changes ↩

@nnethercote nnethercote deleted the rm-RunCompiler-emitter branch October 18, 2022 20:21
@eddyb
Copy link
Member

eddyb commented Nov 30, 2022

Please search for public APIs you want to remove/change before doing so.

There was no need for this "cleanup", it's just a change for the sake of change, and now some level of control has been removed from custom drivers. Maybe all rustc_interface changes like this should go through MCPs?

It looks like there's no way to capture the compiler output without abusing std::io::set_output_capture, and even that likely doesn't apply to the compiler because it seems like std::io::{stdout,stderr} bypass output capture (and only print!/eprint!/panic! gets captured).

Might be easier to try to create a Session {...} by hand, even though that feels somewhat daunting, hopefully most of it is unnecessary (since this is just a test driver).

(Not expecting this to get reverted, hence the workarounds, but it would be nice to not have repeats in the future)

@bjorn3
Copy link
Member

bjorn3 commented Nov 30, 2022

Please search for public APIs you want to remove/change before doing so.

Should have done that. My bad. @nnethercote do you want to revert this PR and add a comment about it's use?

@eddyb eddyb mentioned this pull request Nov 30, 2022
Merged
@eddyb
Copy link
Member

eddyb commented Nov 30, 2022 •
edited
Loading

We've already worked around it, don't worry about a revert:

Turns out that I got turned around and Session {...} is impossible because that function is in rustc_session itself, but we were still able to find a way to create an artificial Session and still mutate it afterwards.

Like I said before, the more important point is that more care should be taken in the future.

EDIT: btw I opened an issue for us moving off of these weird unit tests:

LegNeato added a commit to LegNeato/rust that referenced this pull request Aug 30, 2023
@LegNeato
Make termcolor types public in rustc_errors
da86cdf 
After rust-lang#114104, `rust-gpu` is unable to create a custom `Emitter` as the bounds have changed to include `WriteColor`.

I was able to work around this by adding `termcolor` as a direct dependency, but I believe this should be exposed as part of `rustc_errors` proper.

See rust-lang#102992 for why `rust-gpu` needs to create a custom emitter.
@LegNeato LegNeato mentioned this pull request Aug 30, 2023
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 31, 2023
@matthiaskrgr
Rollup merge of rust-lang#115393 - LegNeato:patch-1, r=oli-obk
82675f6 
Make `termcolor` types public in `rustc_errors`

After rust-lang#114104, `rust-gpu` is unable to create a custom `Emitter` as the bounds have changed to include `WriteColor`.

I was able to work around this by adding `termcolor` as a direct dependency, but I believe this should be exposed as part of `rustc_errors` proper.

See rust-lang#102992 for why `rust-gpu` needs to create a custom emitter.
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

Assignees

@bjorn3 bjorn3

Labels
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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.66.0
Development

Successfully merging this pull request may close these issues.
8 participants
@nnethercote @rust-log-analyzer @bjorn3 @bors @rust-timer @eddyb @rust-highfive @rustbot