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.

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 --crate-type=metadata deprecation warning #42277

Merged
merged 1 commit into from
May 31, 2017

Conversation

citizen428
Copy link
Contributor

Fixes #38640

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@Mark-Simulacrum
Copy link
Member

Looks like there's a warning you'll need to fix:

[00:06:12] error: variable does not need to be mutable
[00:06:12]     --> /checkout/src/librustc/session/config.rs:1634:9
[00:06:12]      |
[00:06:12] 1634 |     let mut emit_metadata = false;
[00:06:12]      |         ^^^^^^^^^^^^^^^^^
[00:06:12]      |
[00:06:12]      = note: #[deny(unused_mut)] implied by #[deny(warnings)]
[00:06:12] note: lint level defined here
[00:06:12]     --> /checkout/src/librustc/lib.rs:23:9
[00:06:12]      |
[00:06:12] 23   | #![deny(warnings)]
[00:06:12]      |         ^^^^^^^^
[00:06:12] 
[00:06:12] error: aborting due to previous error

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 28, 2017
@citizen428
Copy link
Contributor Author

@Mark-Simulacrum I know, thanks. Basically just did that in a rush earlier and pushed it up, figured I'll look at the Travis output later. I can't seem to manage labels, can you change it to S-waiting-on-author please?

@Mark-Simulacrum Mark-Simulacrum 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 May 28, 2017
@citizen428
Copy link
Contributor Author

citizen428 commented May 28, 2017

@Mark-Simulacrum I removed the mut which should fix the warning. I'm just wondering if that variable can be removed completely?

let emit_metadata = false;
...
return Ok((crate_types, emit_metadata));

It's only called in one location in the same file (line 1357) and I think it might be better to completely remove it.

cc @nikomatsakis

@Mark-Simulacrum
Copy link
Member

Yeah, remove emit_metadata here, and if you follow that change through (change the return type of parse_crate_types_from_list and go from there, you'll find that OutputType::Metadata can probably be removed as well (removing a bunch of cruft from the compiler that's no longer needed since this is always false).

@citizen428
Copy link
Contributor Author

citizen428 commented May 29, 2017

Just FYI, I'm working my way through this. Properly removing the flag instead of having it permanently set to false is somewhat more effort than originally outlined in the mentoring description, but seems to be worth it.

@citizen428 citizen428 force-pushed the remove-crate-type-metadata branch 2 times, most recently from 6c34587 to 582d350 Compare May 29, 2017 09:20
@Mark-Simulacrum
Copy link
Member

src/test/run-pass/auxiliary/rmeta_rmeta.rs and src/test/run-pass/rmeta.rs need to be deleted since they are testing --emit=metadata.

@citizen428
Copy link
Contributor Author

@Mark-Simulacrum src/test/run-pass/auxiliary/rmeta_rmeta.rs is already removed as you can see in the file list. But thanks for confirming that they can indeed be remove. There are still a few leftovers like meta_stats etc. that I'm a bit unsure about. If we do this, we might as well rip out everything that's not necessary anymore.

@Mark-Simulacrum
Copy link
Member

Actually, I think I lied; we do want to keep --emit=metadata, it's the crate type that is gone. I think rmeta_meta should've been kept actually, as should at least most of the other tests. Sorry about the run-around. This may mean that some of the code that you've removed needs to be re-added..

@citizen428 citizen428 force-pushed the remove-crate-type-metadata branch 2 times, most recently from a31ec08 to 050a10f Compare May 29, 2017 15:43
@citizen428
Copy link
Contributor Author

@Mark-Simulacrum I went back to a very early version of this PR, basically just removing the code snippet that had the deprecation warning, the related emit_metadata variable and its use at the callsite.

@citizen428
Copy link
Contributor Author

@Mark-Simulacrum This PR is S-waiting-on-review again.

@Mark-Simulacrum Mark-Simulacrum 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 May 30, 2017
@nikomatsakis
Copy link
Contributor

@bors r+ -- thanks @citizen428 !

@bors
Copy link
Contributor

bors commented May 30, 2017

📌 Commit 9873acc has been approved by nikomatsakis

@Mark-Simulacrum
Copy link
Member

@bors rollup

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 31, 2017
…ta, r=nikomatsakis

Remove --crate-type=metadata deprecation warning

Fixes rust-lang#38640
bors added a commit that referenced this pull request May 31, 2017
Rollup of 7 pull requests

- Successful merges: #42126, #42196, #42252, #42277, #42315, #42329, #42330
- Failed merges:
@bors bors merged commit 9873acc into rust-lang:master May 31, 2017
@citizen428 citizen428 deleted the remove-crate-type-metadata branch June 2, 2017 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants