Repository navigation
document safety requirements for atomic intrinsics - #163140
Conversation
rustbot
added
S-waiting-on-review
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| /// | ||
| /// # Safety | ||
| /// | ||
| /// This is equivalent to [Atomic::from_ptr] followed by [Atomic::compare_exchange], |
There was a problem hiding this comment.
If we're going to write these they should specify the full type, via something like <Atomic<T>>::from_ptr - otherwise the alignment safety requirement requires some parsing.
There was a problem hiding this comment.
If we're going to write these they should specify the full type, via something like
<Atomic<T>>::from_ptr- otherwise the alignment safety requirement requires some parsing.
I did some modification about this PR. First, I replaced Atomic::from_ptr with Atomic<T>::from_ptr. I understand the requirement that should specify the full type, but it can only add a concrete type for the doc link (I chose AtomicI32 for unification).
Second, add pointer safety requirement for atomic_cxchg, atomic_cxchgweak and atomic_xchg. And for every place talking about pointer type, I use AtomicPtr<P> to denote that T is a pointer type.
Third, remove the "with ..." part and place it in the beginning.
Thank you for your feedback! If there exist some other wrong discriptions I will fix them.
There was a problem hiding this comment.
If we're going to write these they should specify the full type, via something like
<Atomic<T>>::from_ptr- otherwise the alignment safety requirement requires some parsing.
I did some modification about this PR. First, I replaced Atomic::from_ptr with Atomic<T>::from_ptr. I understand the requirement that should specify the full type, but it can only add a concrete type for the doc link (I chose AtomicI32 for unification).
Second, adding pointer safety requirement for atomic_cxchg, atomic_cxchgweak and atomic_xchg. And for every place talking about pointer type, I use AtomicPtr<P> to denote that T is a pointer type.
Third, removing the "with ..." part and place it in the beginning.
Thank you for your feedback! If there exist some other wrong discriptions I will fix them.
| /// | ||
| /// This is equivalent to [Atomic::from_ptr] followed by [Atomic::compare_exchange], | ||
| /// with the returned `T` being the previous value and the returned `bool` being | ||
| /// whether the exchange was successful. |
There was a problem hiding this comment.
I would remove the "with ..." part (and probably the compare_exchange part, at least from here; if you want it it should be outside the safety section). The safety requirements here are exactly the same as <Atomic<T>>::from_ptr AFAICT.
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
2e2a0aa to
33d8a4d
Compare
|
@rustbot ready |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
33d8a4d to
17557a8
Compare
17557a8 to
99cc8cf
Compare
rust-bors
Bot
added
S-waiting-on-bors
…imulacrum document safety requirements for atomic intrinsics This PR serves as a complement to PR rust-lang#162854 by adding safety docs for the remaining atomic operations. These operations pass raw pointers as arguments and are marked unsafe, so I think it is necessary to explain why they are unsafe. For `atomic_cxchg`, `atomic_cxchgweak`, `atomic_xchg`, these APIs have the same process whether `T` is an integer or a pointer type. For `atomic_xadd`, `atomic_xsub`, `atomic_and`, `atomic_or`, `atomic_xor`, these APIs correspond to different operations in `core/src/sync/atomic.rs` for `T` is an integer or a pointer type. For `atomic_max`, `atomic_min`, `atomic_umax`, `atomic_umin`, they only process when `T` is an integer (signed or unsigned) type. For `atomic_nand`, there doesn't exist a method in `AtomicPtr<T>`, so I don't link the related method.
Rollup of 18 pull requests Successful merges: - #158102 (When compiling without a specified `--edition`, emit a message) - #162027 (std: add `fs::rename_noreplace`) - #162761 (Lower attributes for functions without bodies) - #163161 (implement FCW for `rustc_allowed_through_unstable_modules` items) - #163613 (Tweak the rendering of "not general enough" errors on the old trait solver) - #162062 (core: fix the docs of PanicInfo::location) - #163140 (document safety requirements for atomic intrinsics) - #163342 (Don't imply incorrect things about `Global` in the docs of `System`) - #163445 (Add safety comments for alloc::str) - #163503 (Mark Rc strong/weak count methods must_use) - #163548 (fs::set_permissions_nofollow: Android support, test cleanup) - #163585 ([triagebot] Create `debugger_visualizer` assign group) - #163597 (Add `SplitPathsRef` implementation for motor to make std build) - #163602 (Move media & home dirs tests to fs tests.) - #163667 (Finalize changes on expect messages for library/core/src/fmt/mod.rs) - #163682 ([rustdoc] Correctly link to (imported) enum variants with "jump to def") - #163683 (Fix GCC codegen backend comment in bootstrap) - #163703 (Move more `rustdoc-html tests` in the right location) Failed merges: - #161491 (Rip out old solver coherence)
Rollup of 18 pull requests Successful merges: - #158102 (When compiling without a specified `--edition`, emit a message) - #162761 (Lower attributes for functions without bodies) - #163161 (implement FCW for `rustc_allowed_through_unstable_modules` items) - #163613 (Tweak the rendering of "not general enough" errors on the old trait solver) - #162062 (core: fix the docs of PanicInfo::location) - #163140 (document safety requirements for atomic intrinsics) - #163342 (Don't imply incorrect things about `Global` in the docs of `System`) - #163445 (Add safety comments for alloc::str) - #163503 (Mark Rc strong/weak count methods must_use) - #163548 (fs::set_permissions_nofollow: Android support, test cleanup) - #163585 ([triagebot] Create `debugger_visualizer` assign group) - #163597 (Add `SplitPathsRef` implementation for motor to make std build) - #163602 (Move media & home dirs tests to fs tests.) - #163667 (Finalize changes on expect messages for library/core/src/fmt/mod.rs) - #163682 ([rustdoc] Correctly link to (imported) enum variants with "jump to def") - #163683 (Fix GCC codegen backend comment in bootstrap) - #163703 (Move more `rustdoc-html tests` in the right location) - #163725 (some crashes fixed with next-solver) Failed merges: - #161491 (Rip out old solver coherence)
Rollup merge of #163140 - yilin0518:fix_atomic_ops, r=Mark-Simulacrum document safety requirements for atomic intrinsics This PR serves as a complement to PR #162854 by adding safety docs for the remaining atomic operations. These operations pass raw pointers as arguments and are marked unsafe, so I think it is necessary to explain why they are unsafe. For `atomic_cxchg`, `atomic_cxchgweak`, `atomic_xchg`, these APIs have the same process whether `T` is an integer or a pointer type. For `atomic_xadd`, `atomic_xsub`, `atomic_and`, `atomic_or`, `atomic_xor`, these APIs correspond to different operations in `core/src/sync/atomic.rs` for `T` is an integer or a pointer type. For `atomic_max`, `atomic_min`, `atomic_umax`, `atomic_umin`, they only process when `T` is an integer (signed or unsigned) type. For `atomic_nand`, there doesn't exist a method in `AtomicPtr<T>`, so I don't link the related method.
4 participants
This PR serves as a complement to PR #162854 by adding safety docs for the remaining atomic operations. These operations pass raw pointers as arguments and are marked unsafe, so I think it is necessary to explain why they are unsafe.
For
atomic_cxchg,atomic_cxchgweak,atomic_xchg, these APIs have the same process whetherTis an integer or a pointer type.For
atomic_xadd,atomic_xsub,atomic_and,atomic_or,atomic_xor, these APIs correspond to different operations incore/src/sync/atomic.rsforTis an integer or a pointer type.For
atomic_max,atomic_min,atomic_umax,atomic_umin, they only process whenTis an integer (signed or unsigned) type.For
atomic_nand, there doesn't exist a method inAtomicPtr<T>, so I don't link the related method.