-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Update the description of the ticket to point at RFC 1721 #70576
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? @rust-lang/compiler (I wish we had a reassignment command) |
@Rustin-Liu I think the text should be changed to actually point out what's unstable. The link you added can be added to the tracking issue #37406 instead (someone with editing permissions can add it). |
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 |
d74f5b5
to
0d3d9cf
Compare
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 |
@estebank could you please take a look? |
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.
@Rustin-Liu: after the suggested change, could you flatten your commits into one? Then everything looks good! I think we should be pointing to the tracking issue, but RFC 1721 should be more prominently displayed: I'll try to sort that out.
src/librustc_metadata/native_libs.rs
Outdated
&self.tcx.sess.parse_sess, | ||
sym::link_cfg, | ||
span.unwrap(), | ||
"kind=\"link_cfg\" is unstable, see https://github.com/rust-lang/rfcs/pull/1721", |
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 we need to mention rust-lang/rfcs#1721 here — we already mention the relevant issue below. However, #37406 seems to be closed, so maybe we'll need to open it.
"kind=\"link_cfg\" is unstable, see https://github.com/rust-lang/rfcs/pull/1721", | |
"kind=\"link_cfg\" is unstable", |
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.
Thanks for your review. Removed.
Fmt code Update tests Modify msg Signed-off-by: Rustin-Liu <[email protected]>
8a566e2
to
d2ad9f4
Compare
@Rustin-Liu: perfect, thank you! @bors r+ rollup |
📌 Commit d2ad9f4 has been approved by |
Rollup of 9 pull requests Successful merges: - rust-lang#69860 (Use associated numeric consts in documentation) - rust-lang#70576 (Update the description of the ticket to point at RFC 1721) - rust-lang#70597 (Fix double-free and undefined behaviour in libstd::syn::unix::Thread::new) - rust-lang#70640 (Hide `task_context` when lowering body) - rust-lang#70641 (Remove duplicated code in trait selection) - rust-lang#70707 (Remove unused graphviz emitter) - rust-lang#70720 (Place TLS initializers with relocations in .tdata) - rust-lang#70735 (Clean up E0502 explanation) - rust-lang#70741 (Add test for rust-lang#59023) Failed merges: r? @ghost
Fixes #70538.
My first PR to rust. So please let me know if I'm doing something wrong.