-
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
Prohibit global_allocator
in submodules
#51335
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 |
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 reviewer is currently away. Maybe someone else from @rust-lang/compiler can review this? |
@bors r+ |
📌 Commit 461d3e0 has been approved by |
🔒 Merge conflict |
@mark-i-m you need to rebase on top of the latest master. |
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 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_allocator/expand.rs
Outdated
self.in_submod += 1; | ||
let ret = fold::noop_fold_mod(m, self); | ||
self.in_submod -= 1; | ||
info!("exit submodule"); |
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 don't think info!
-level debug logging should be kept in. You can use debug!
, if you want.
r? @oli-obk Time for another review? |
@bors r+ |
📌 Commit d264e25b730e598562da471723c8b7647c810a4e has been approved by |
??? I just rebased this? |
☔ The latest upstream changes (presumably #51726) made this pull request unmergeable. Please resolve the merge conflicts. |
- we need to figure out hygiene first - change the test to check that the prohibition works with a good error msg - leaves some comments and debugging code - leaves some of our supposed fixes
Rebased again |
@bors r+ |
📌 Commit 16d7f87 has been approved by |
Let's avoid rebasing this again. @bors p=1 |
Prohibit `global_allocator` in submodules Background: #44113 is caused by weird interactions with hygiene. Hygiene is hard. After a lot of playing around, we decided that the best path forward would be to prohibit `global_allocator`s from being in submodules for now. When somebody gets it working, we can re-enable it. This PR contains the following - Some hygiene "fixes" -- things I suspect are the correct thing to do that will make life easier in the future. This includes using call_site hygiene for the generated module and passing the correct crate name to the expansion config. - Comments and minor formatting fixes - Some debugging code - Code to prohibit `global_allocator` in submodules - Test checking that the proper error occurs. cc #44113 #49320 #51241 r? @alexcrichton
☀️ Test successful - status-appveyor, status-travis |
Reason for the beta nomination: global allocators will be stabilized in 1.28, and it's best to prohibit something before it reaches stable. |
@pietroalbini is there something I need to do on my end? |
@mark-i-m no, the compiler team has a meeting later today when they'll decide if this can be backported. |
[beta] Rollup backports Merged and approved: * #51725: Do not build LLVM tools for any of the tools * #51852: Don't use `ParamEnv::reveal_all()` if there is a real one available * #51686: yet another "old borrowck" bug around match default bindings * #51868: Remove process::id from 'Stabilized APIs' in 1.27.0 release notes * #51335: Prohibit `global_allocator` in submodules r? @ghost
Background: #44113 is caused by weird interactions with hygiene. Hygiene is hard. After a lot of playing around, we decided that the best path forward would be to prohibit
global_allocator
s from being in submodules for now. When somebody gets it working, we can re-enable it.This PR contains the following
global_allocator
in submodulescc #44113 #49320 #51241
r? @alexcrichton