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.

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

Implement RefUnwindSafe for atomic types #37178

Merged
merged 1 commit into from
Nov 1, 2016
Merged

Implement RefUnwindSafe for atomic types #37178

merged 1 commit into from
Nov 1, 2016

Conversation

apasel422
Copy link
Contributor

Closes #37136

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton alexcrichton added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 14, 2016
@alexcrichton
Copy link
Member

@bors: r+

cc @rust-lang/libs

@bors
Copy link
Contributor

bors commented Oct 14, 2016

📌 Commit da3b0b8 has been approved by alexcrichton

@@ -231,6 +231,34 @@ impl<T: ?Sized> RefUnwindSafe for Mutex<T> {}
#[stable(feature = "unwind_safe_lock_refs", since = "1.12.0")]
impl<T: ?Sized> RefUnwindSafe for RwLock<T> {}

#[stable(feature = "unwind_safe_atomic_refs", since = "1.14.0")]
impl RefUnwindSafe for atomic::AtomicIsize {}
#[stable(feature = "unwind_safe_atomic_refs", since = "1.14.0")]
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't make much sense for this to be marked stable when AtomicI8 is still unstable. I think for the unstable types this should be #[unstable(feature = "integer_atomics", issue = "32976")] like the others impls for these types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Updated.

@apasel422
Copy link
Contributor Author

Should the impls be placed behind #[cfg(target_has_atomic = "x")]?

@alexcrichton
Copy link
Member

@bors: r-

Should the impls be placed behind

Ah yes, indeed!

@apasel422
Copy link
Contributor Author

As a side note, there are portions of the standard library that use atomics without the #[cfg(target_has_atomic = "x")] (most notably Arc and sync::mpsc). Should they be defined conditionally as well? Or does std assume/require their existence?

(Obviously not a change to make in this PR.)

@alexcrichton
Copy link
Member

As a side note, there are portions of the standard library that use atomics without the #[cfg(target_has_atomic = "x")]

True yeah, this basically just means that libstd won't compile if AtomicUsize isn't available (intentional).

This seems like a mildly contentious topic on the associated issue, so I'm instead going to...

@rfcbot fcp merge

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 17, 2016
@rfcbot
Copy link

rfcbot commented Oct 17, 2016

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot
Copy link

rfcbot commented Oct 31, 2016

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @alexcrichton, I wasn't able to add the final-comment-period label, please do so.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 31, 2016

📌 Commit f832203 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Nov 1, 2016

⌛ Testing commit f832203 with merge 73f5cad...

bors added a commit that referenced this pull request Nov 1, 2016
Implement `RefUnwindSafe` for atomic types

Closes #37136
@bors bors merged commit f832203 into rust-lang:master Nov 1, 2016
@apasel422 apasel422 deleted the issue-37136 branch November 1, 2016 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants