-
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
Allow CompilerControllers to access rustc_plugin::registry::Registry #36240
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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. |
@@ -97,7 +97,7 @@ pub fn compile_input(sess: &Session, | |||
} | |||
}; | |||
|
|||
let krate = { | |||
let (krate,registry) = { |
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.
nit: there should be a space after the comma
@leeopop Looks good to me modulo comments. n.b. If you add "fixes #36064" to your initial comment (#36240 (comment)), #36064 will be closed automatically when this PR lands. |
Thanks for your comments. I just applied them. |
r? @jseyfried |
@@ -379,6 +381,9 @@ impl<'a, 'b, 'ast, 'tcx> CompileState<'a, 'b, 'ast, 'tcx> { | |||
cstore: &'a CStore) | |||
-> CompileState<'a, 'b, 'ast, 'tcx> { | |||
CompileState { | |||
//Registry initialization should go before the krate initialization | |||
//because krate: Some(krate) needs move. |
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.
formatting nit: there should be a space following //
, e.g. // Registry initialization ...
.
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 this comment is needed, though -- if someone tries to move krate: Some(krate)
above registry: ...
for some reason, they'll get the same error you got.
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.
or if you want a comment, I would write it: // Initialize the registry before moving
krate``.
@leeopop r=me with formatting nit addressed. |
r= @jseyfried |
📌 Commit ca5dfd0 has been approved by |
Allow CompilerControllers to access rustc_plugin::registry::Registry fixes #36064 I chose to put ructc_plugin::registry::Registry structure into CompilerState structure, instead of Session structure. This will preserve dependencies among librustc, libructc_driver, and libructc_plugin. @jseyfried @sanxiyn
fixes #36064
I chose to put ructc_plugin::registry::Registry structure
into CompilerState structure, instead of Session structure.
This will preserve dependencies among librustc, libructc_driver, and libructc_plugin.
@jseyfried @sanxiyn