Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Experiment: Apply #[deprecated_safe] to env::set_var/env::remove_var #95942

Closed
wants to merge 8 commits into from
Closed

Experiment: Apply #[deprecated_safe] to env::set_var/env::remove_var #95942

wants to merge 8 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 11, 2022

cc #94978
cc #95025
cc #94619

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 11, 2022
@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rust-highfive
Copy link
Collaborator

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 11, 2022
@ghost
Copy link
Author

ghost commented Apr 11, 2022 •
edited by ghost
Loading

@lcnr, i didn't think a draft PR would pick a reviewer 😅 this is just an experiment (so far) to see what applying #[deprecated_safe] within libstd might look like

the PR this is based on (#95025) hasn't landed yet and that's where most of the commits come from

@ghost ghost mentioned this pull request Apr 12, 2022
Closed
3 tasks
@lcnr
Copy link
Contributor

lcnr commented Apr 12, 2022

will be away next week, so if it gets ready for review by that time, feel free to reassign this, will needs libs-api signoff anyways

@lcnr lcnr added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 12, 2022
@bors
Copy link
Contributor

bors commented Apr 15, 2022

☔ The latest upstream changes (presumably #96087) made this pull request unmergeable. Please resolve the merge conflicts.

riking
riking reviewed Apr 21, 2022
View reviewed changes
compiler/rustc_driver/src/lib.rs
@@ -1320,7 +1324,11 @@ pub fn main() -> ! {
init_rustc_env_logger();
signal_handler::install();
let mut callbacks = TimePassesCallbacks::default();
install_ice_hook();
// SAFETY: in main(), no other threads could be reading or writing the environment
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// SAFETY: Lack of thread conflicts asserted by the caller

library/test/src/term/terminfo/searcher/tests.rs
@@ -13,7 +13,11 @@ fn test_get_dbpath_for_term() {
}
assert!(x("screen") == "/usr/share/terminfo/s/screen");
assert!(get_dbpath_for_term("") == None);
// FIXME(skippy) there's no fix for deprecated_safe until tests can be run single threaded
#[cfg_attr(not(bootstrap), allow(deprecated_safe))]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This style of cfg_attrs is suspicious - aren't the bootstrap cfgs going to be deleted at the next upgrade?

Instead, use // SAFETY: FIXME(skippy) This code is incorrect.

@CAD97
Copy link
Contributor

CAD97 commented Jul 4, 2022

Aside: before_exec should probably also be marked #[deprecated_safe].

Repository owner closed this by deleting the head repository Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lcnr lcnr

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@rust-highfive @lcnr @bors @CAD97 @riking @ghost @rustbot @User1785604260