do not reference LLVM for our concurrency memory model #65339
Conversation
|
r? @KodrAus (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
src/libcore/sync/atomic.rs
Outdated
| @@ -18,11 +18,11 @@ | |||
| //! | |||
| //! Each method takes an [`Ordering`] which represents the strength of | |||
| //! the memory barrier for that operation. These orderings are the | |||
| //! same as [LLVM atomic orderings][1]. For more information see the [nomicon][2]. | |||
| //! same as the [C++ atomic orderings][1]. For more information see the [nomicon][2]. | |||
There was a problem hiding this comment.
AFAIK C just copies C++ here. And there were some fixes since C++11.
But indeed the Nomicon says "C11"...
There was a problem hiding this comment.
Yea; I mostly want to have a consistent saying one way or the other. (I don't have a preference re. which place should be updated and you are the field expert. Tho cc @nikomatsakis )
There was a problem hiding this comment.
(Adding Cc in an edit does not trigger email notifications.)
The C page on memory_order is much less informative, so I'd prefer to link to the C++ page. And that page currently reflects C++20. I am not sure if it is worth trying to pin a page from some older C++ version; AFAIK all the changes have been bugfixes that we certainly want.
I assume @Gankra wrote "C11" as most people just say "C11 concurrency". It's shorter to write than "C/C++ from 2011 and later".
There was a problem hiding this comment.
I could replace "C++" by "C/C++" everywhere, and we could do the same in the Nomicon?
There was a problem hiding this comment.
Done, also see rust-lang/nomicon#168.
There was a problem hiding this comment.
This is a minor thing, but I'm glad to see this fixed, I've been correcting Gankro on this since at least three years ago.
(I suspect the people using "C11 concurrency" are exactly those who read it from Nomicon, because I very rarely see it used outside of Rust community.)
There was a problem hiding this comment.
@petrochenkov so do you think we should further clarify the comment I added on this in the Nomicon?
There was a problem hiding this comment.
It's not just Gankra though; here's a talk by some of my colleagues also using "C11". I think it is generally understood as a short-hand for "C/C++11" -- but it is indeed somewhat confusing.
There was a problem hiding this comment.
I don't know, looks ok as is.
|
r? @Centril |
|
Thanks ❤️ @bors r+ rollup |
|
📌 Commit f363550 has been approved by |
bors
added
S-waiting-on-bors
This comment has been minimized.
This comment has been minimized.
|
@bors r=Centril |
|
📌 Commit d6ab45d has been approved by |
do not reference LLVM for our concurrency memory model Fixes rust-lang#65282
Rollup of 13 pull requests Successful merges: - #65039 (Document missing deny by default lints) - #65069 (Implement Clone::clone_from for VecDeque) - #65165 (Improve docs on some char boolean methods) - #65248 (Suggest `if let` on `let` refutable binding) - #65250 (resolve: fix error title regarding private constructors) - #65295 (Move diagnostics code out of the critical path) - #65320 (Report `CONST_ERR` lint in external macros) - #65327 (replace the hand-written binary search with the library one) - #65339 (do not reference LLVM for our concurrency memory model) - #65357 (syntax: simplify maybe_annotate_with_ascription) - #65358 (simplify maybe_stage_features) - #65359 (simplify integer_lit) - #65360 (mbe: reduce panictry! uses.) Failed merges: r? @ghost
Fixes #65282