-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 some code to librustc_passes. #67688
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/librustc/lib.rs
Outdated
@@ -102,7 +102,30 @@ pub mod middle { | |||
pub mod exported_symbols; | |||
pub mod free_region; | |||
pub mod lang_items; | |||
pub mod lib_features; | |||
pub mod lib_features { |
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.
Maybe split this out to middle.rs
?
cc @Centril |
|
||
use rustc_error_codes::*; | ||
use lazy_static::lazy_static; | ||
|
||
macro_rules! weak_lang_items { |
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.
Any particular reason you where unable to move the entire file?
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.
librustc_codegen_ssa
depends on the link_name
function. I did not want to introduce a dependency on librustc_passes
just for that.
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'd just add that dependency instead of keeping stuff in rustc.
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.
Please, don't introduce the dependency, it's a much larger build time penalty than a tiny function in rustc.
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.
That is not true though.
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.
Maybe make a rustc_lang_items
and put that as a librustc_codegen_ssa
dependency instead?
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.
rustc_lang_items
is probably sensible long term, also because I'd like to be able to refer to lang items in lowering (#60607).
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.
Let's just move these to a new rustc_lang_items
crate then.
The first 4 commits look good, you could split them off into a separate PR so I can approve them. I'd like the |
👍 -- this tends to work well with git so as to preserve blame and history. |
Rebased, with the split-out commits removed. |
Maybe move the non lang-items commits to #67698 too, they should be good to go. |
Move reachable_set and diagnostic_items to librustc_passes. Split out of rust-lang#67688 r? @Zoxc
☔ The latest upstream changes (presumably #67721) made this pull request unmergeable. Please resolve the merge conflicts. |
Move the region_scope_tree query to librustc_passes. Split out of rust-lang#67688. r? @Zoxc
Move the region_scope_tree query to librustc_passes. Split out of rust-lang#67688. r? @Zoxc
☔ The latest upstream changes (presumably #67700) made this pull request unmergeable. Please resolve the merge conflicts. |
I see you added some more stuff here. |
☔ The latest upstream changes (presumably #67886) made this pull request unmergeable. Please resolve the merge conflicts. |
@cjgillot can you rebase this? thanks |
Rebased. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☔ The latest upstream changes (presumably #68330) made this pull request unmergeable. Please resolve the merge conflicts. |
Ping from triage: |
Rebased with lang_items changes moved to #68554. |
src/librustc_passes/check_attr.rs
Outdated
ForeignStatic, | ||
ForeignTy, | ||
pub(crate) trait TargetExt { | ||
fn from_impl_item<'tcx>(tcx: TyCtxt<'tcx>, impl_item: &hir::ImplItem<'_>) -> Target; |
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 think this method can just stay in rustc
.
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.
This method is only used in this file. Since I plan to remove rustc::hir::check_attr
in #68554, I think I will make it a free function.
☔ The latest upstream changes (presumably #68635) made this pull request unmergeable. Please resolve the merge conflicts. |
@bors r+ |
📌 Commit f9335e9 has been approved by |
☀️ Test successful - checks-azure |
Split lang_items to crates `rustc_hir` and `rustc_passes`. As discussed in comment rust-lang#67688 (comment)
Per instructions by @Zoxc, some code from
librustc::middle
is moved tolibrustc_passes
.cc #65031
r? @Zoxc