-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Move rustc_back modules where they belong. #46305
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (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. |
r? @alexcrichton What should be done about the tests that use |
src/librustc_trans/lib.rs
Outdated
@@ -66,6 +66,7 @@ extern crate rustc_errors as errors; | |||
extern crate serialize; | |||
#[cfg(windows)] | |||
extern crate cc; // Used to locate MSVC | |||
pub extern crate tempdir; |
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.
why "pub extern crate"? Shoulldn't this import be private?
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.
Ah ok you are exporting it for tests so that they don't need to use Cargo. Could you add a comment to that effect?
@@ -12,11 +12,11 @@ | |||
|
|||
#![feature(rustc_private)] |
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.
I suppose you can remove this test if we're moving to tempdir
from crates.io
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.
The reason as to why we don't change or remove the tests it's because the tests just happen to use tempdir
, these don't test tempdir
itself.
☔ The latest upstream changes (presumably #44884) made this pull request unmergeable. Please resolve the merge conflicts. |
The slice stuff is available in libcore now (#45306). |
Looks pretty good to me, thanks! You can also import #![feature(rustc_private)]
extern crate tempdir;
// ... |
@petrochenkov Oh, great, I didn't notice. If the @alexcrichton We tried that and it wasn't found, unlike the |
@eddyb that means that the test wasn't set up correctly or something like that, the crate exists and it can be linked to. |
@alexcrichton it looks like new commits have been pushed! |
@shepmaster There's still more we want to do here (at the very least #46305 (comment)), but @irinagpopa won't be able to get back to it until next week. |
Ah, no point in waiting for more, at least it's self-contained and @bors r=alexcrichton,eddyb |
📌 Commit 2c175df has been approved by |
Move rustc_back modules where they belong.
☀️ Test successful - status-appveyor, status-travis |
No description provided.