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

Stabilize volatile read and write #1467

Merged
merged 2 commits into from
Feb 18, 2016
Merged

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Jan 18, 2016

This stabilizes the volatile_load and volatile_store intrinsics as ptr::read_volatile and ptr::write_volatile.

Rendered

@nrc nrc added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Jan 18, 2016
@nagisa
Copy link
Member

nagisa commented Jan 19, 2016

I think this RFC proposes to do an obviously right change. There’s no compatibility hazard I can think of and the functions are very useful in certain cases.

👍

@shepmaster
Copy link
Member

I'd be happy to see these made stable, but are there any environments where these functions can be used without requiring other unstable things? Said another way, would stabilizing these functions make it so that some project can switch from nightly to stable?

I know that doesn't need to be a requirement, and we have to start somewhere.

@nikomatsakis
Copy link
Contributor

Seems like a good idea to me.

@alexcrichton
Copy link
Member

As a bit of a bikeshed I might prefer to rename these to read_volatile and write_volatile (to group nicely with the existing read and write), but otherwise this seems fine to me. I believe it meshes well with the rest of the functions in the std::ptr module in terms of naming and signature.

I think the biggest question in terms of stabilization is what does volatile mean, and I agree with this RFC that it's well defined in C and other languages, so I would be fine having documentation redirecting to those relevant documents as well. I would perhaps feel best if we could hash out in this RFC exactly what semantics we'd point to (e.g. what documentation we'd point to) just to be extra sure it's ok, but that's not necessarily required in my opinion.

In the long run I think we'll eventually want a wrapper like Volatile<T> or something like that, but I'm more than ok deferring that and only providing the bare bones in the standard library to start out with (e.g. these functions). I think that you're able to build up everything else on top of these functions.

@alexcrichton alexcrichton self-assigned this Jan 21, 2016
@Amanieu
Copy link
Member Author

Amanieu commented Jan 22, 2016

I renamed the functions to read_volatile/write_volatile and added a link to the LLVM definition of volatile.

@alexcrichton
Copy link
Member

🔔 This RFC is now entering its week-long final comment period 🔔


My own personal opinion is that this is good to go!

@alexcrichton alexcrichton added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Feb 11, 2016
@briansmith
Copy link

Please do not cite the LLVM documentation as the reference for the semantics. LLVM's documentation doesn't describe standard C semantics, but rather some semantics that are stronger, AFAICT, than what ISO C requires. If Rust wants to be different from ISO C11 then it would be better for Rust to specify what semantics it wants to have, and then verify whether LLVM implements (at least) the wanted semantics. The way this RFC looks now, it will be basically impossible to implement Rust without using LLVM, without reverse engineering LLVM.

@huonw
Copy link
Member

huonw commented Feb 13, 2016

@briansmith note that this won't be the first Rust feature that is currently defined in terms of LLVM, nor do RFCs always have be complete specifications (i.e. we may want to learn more about how people want to use a feature before nailing down its semantics completely). Of course, having existing un(der)specified features doesn't automatically imply that we should add more. That said, I would expect documentation to not link to LLVM, but rather give some vague description about approximately what the semantics are (e.g. "volatile reads/writes are never elided nor reordered with other volatile reads/writes"), at least until Rust actually has a spec.

@alexcrichton
Copy link
Member

The libs team discussed this RFC during triage yesterday and the conclusion was to merge, so I will do so! Thanks again for the RFC @Amanieu!

@alexcrichton alexcrichton merged commit 7f83957 into rust-lang:master Feb 18, 2016
bors added a commit to rust-lang/rust that referenced this pull request Feb 21, 2016
Tracking issue: #31756
RFC: rust-lang/rfcs#1467

I've made these unstable for now. Should they be stabilized straight away since we've had plenty of experience with people using the unstable intrinsics?
@Centril Centril added A-raw-pointers Proposals relating to raw pointers. A-volatile Proposals related to volatile. labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-raw-pointers Proposals relating to raw pointers. A-volatile Proposals related to volatile. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants