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

Make AtomicBool the same size as bool #32365

Closed
wants to merge 1 commit into from

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Mar 20, 2016

This allows AtomicBool to be transmuted to a bool, which makes it more consistent with the other atomic types. Note that this now guarantees that the atomic type will always contain a valid bool value, which wasn't the case before (due to fetch_nand).

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

@nagisa
Copy link
Member

nagisa commented Mar 20, 2016

The impl looks okay to me. The comment regarding CAS loop also seems to be true, and LLVM won’t accept something like atomicrmw nand i1* %2, i1 0 [ordering] anyway. I also like the cleanup made possible around every other function.

@alexcrichton
Copy link
Member

I'd be a little nervous to land this ahead of rust-lang/rfcs#1543 and rust-lang/rfcs#1505, but it seems fine to me in principle.

I'm tagging with T-libs so this comes up during triage, but I suspect we'll merge in any case :)

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

The libs team discussed this during triage yesterday, and some of our discussion led to some surprising conclusions, but in general we felt a little uncomfortable merging this ahead of rust-lang/rfcs#1543. In light of that, we're thinking that we may want to hold off on this for now until we move on one of those RFCs

@alexcrichton
Copy link
Member

Ok, we discussed this again in light of accepting rust-lang/rfcs#1543, but we concluded that we probably don't want to do this at this time due to the uncertainty about what to do with platform-specific types. We can certainly revisit this though once the types become stable!

@Amanieu
Copy link
Member Author

Amanieu commented Apr 21, 2016

I don't think platform-specific types are relevant here since any platform that supports pointer-sized atomics will always support byte-sized atomics. Changing the size of AtomicBool to 1 byte will therefore not introduce any compatibility issues.

bors added a commit that referenced this pull request May 9, 2016
Add integer atomic types

Tracking issue: #32976
RFC: rust-lang/rfcs#1543

The changes to AtomicBool in the RFC are not included, they are in a separate PR (#32365).
bors added a commit that referenced this pull request May 14, 2016
Make AtomicBool the same size as bool

Reopening #32365

This allows `AtomicBool` to be transmuted to a `bool`, which makes it more consistent with the other atomic types. Note that this now guarantees that the atomic type will always contain a valid `bool` value, which wasn't the case before (due to `fetch_nand`).

r? @alexcrichton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants