Adding links for Atomics docs #29377 #40871

projektir · 2017-03-28T04:00:21Z

This should be good for std::sync::atomic. The other pages still need more (examples, etc.).

rust-highfive · 2017-03-28T04:00:29Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

steveklabnik

Thank you so much! I have some small nits to make the formatting fall in line with some of our conventions, and one bugfix. 🎊

steveklabnik · 2017-03-28T13:18:30Z

src/libcore/sync/atomic.rs

 //! atomically-reference-counted shared pointer).
 //!
+//! [`Sync`]: ../../marker/trait.Sync.html
+//! [arc]: ../struct.Arc.html


https://travis-ci.org/rust-lang/rust/jobs/215789767#L8942 (It says this link is broken.)

The reason for this is that these docs also get rendered for libcore, not just libstd. So can you change it to ../../../std/sync/struct.Arc.html? Tricky!

Yeah, I had some issues with this one. I wasn't sure if a longer path was better for this to get linked in both place correctly. I'll play around it some more.

For now it is, yeah. Someday...

steveklabnik · 2017-03-28T13:20:36Z

src/libcore/sync/atomic.rs

@@ -149,21 +159,26 @@ unsafe impl<T> Sync for AtomicPtr<T> {}
 #[derive(Copy, Clone, Debug)]
 pub enum Ordering {
    /// No ordering constraints, only atomic operations. Corresponds to LLVM's
-    /// `Monotonic` ordering.
+    /// [`Monotonic`][1] ordering.
+    /// [1]: http://llvm.org/docs/Atomics.html#monotonic


While you're here, would you mind helping with a minor formatting issue as well? This should look like this:

/// No ordering constraints, only atomic operations. /// /// Corresponds to LLVM's [`Monotonic`] ordering. /// /// [`Monotonic`]: http://llvm.org/docs/Atomics.html#monotonic

Thanks 😄

steveklabnik · 2017-03-28T13:22:04Z

src/libcore/sync/atomic.rs

    /// on the same value.
+    /// [1]: http://llvm.org/docs/Atomics.html#acquire


Similar here, could this be

/// to the other threads that perform a load with [`Acquire`] ordering /// on the same value. /// /// [`Aquire`]: http://llvm.org/docs/Atomics.html#acquire

steveklabnik · 2017-03-28T13:22:35Z

src/libcore/sync/atomic.rs

    /// in other threads.
+    /// [1]: http://llvm.org/docs/Atomics.html#release


steveklabnik · 2017-03-28T13:23:34Z

src/libcore/sync/atomic.rs

+    /// When coupled with a load, uses [`Acquire`][1] ordering, and with a store
+    /// [`Release`][2] ordering.
+    /// [1]: http://llvm.org/docs/Atomics.html#acquire
+    /// [2]: http://llvm.org/docs/Atomics.html#release


and here:

/// When coupled with a load, uses [`Acquire`] ordering, and with a store /// [`Release`] ordering. /// /// [`Acquire`]: http://llvm.org/docs/Atomics.html#acquire /// [`Release`]: http://llvm.org/docs/Atomics.html#release

steveklabnik · 2017-03-28T13:24:31Z

src/libcore/sync/atomic.rs

@@ -250,7 +266,7 @@ impl AtomicBool {
    ///
    /// [`Ordering`]: enum.Ordering.html
    /// [`Release`]: enum.Ordering.html#variant.Release
-    /// [`AcqRel`]: enum.Ordering.html#variant.Release
+    /// [`AcqRel`]: enum.Ordering.html#variant.AcqRel


nice catch! 😓

steveklabnik · 2017-03-28T13:24:40Z

src/libcore/sync/atomic.rs

@@ -298,7 +317,7 @@ impl AtomicBool {

    /// Stores a value into the bool, returning the old value.
    ///
-    /// `swap` takes an [`Ordering`] argument which describes the memory ordering
+    /// `swap()` takes an [`Ordering`] argument which describes the memory ordering


steveklabnik · 2017-03-28T13:24:44Z

src/libcore/sync/atomic.rs

@@ -324,7 +343,7 @@ impl AtomicBool {
    /// The return value is always the previous value. If it is equal to `current`, then the value
    /// was updated.
    ///
-    /// `compare_and_swap` also takes an [`Ordering`] argument which describes the memory
+    /// `compare_and_swap()` also takes an [`Ordering`] argument which describes the memory


and here (I'll stop now, but all the others too)

steveklabnik · 2017-03-28T13:25:03Z

src/libcore/sync/atomic.rs

    /// ordering of this operation. The first describes the required ordering if the operation
    /// succeeds while the second describes the required ordering when the operation fails. The
    /// failure ordering can't be [`Release`] or [`AcqRel`] and must be equivalent or
    /// weaker than the success ordering.
    ///
+    /// [`compare_exchange()`]: #method.compare_exchange


when removing the ()s this one will have to go too

steveklabnik · 2017-03-28T13:25:34Z

src/libcore/sync/atomic.rs

+            ///
+            /// [`Acquire`]: enum.Ordering.html#variant.Acquire
+            /// [`AcqRel`]: enum.Ordering.html#variant.AcqRel
+            ///


could you remove this extra line please?

projektir · 2017-03-29T22:58:58Z

@steveklabnik I've pushed a new commit for the formatting changes.

steveklabnik

@bors: r+ rollup

thanks so much!

steveklabnik · 2017-03-31T18:31:04Z

Oops, looks like @bors doesn't see review comments.

@bors: r+ rollup

bors · 2017-03-31T18:31:05Z

📌 Commit 4ea03c8 has been approved by steveklabnik

@steveklabnik

Adding links for Atomics docs rust-lang#29377 r? @steveklabnik This should be good for `std::sync::atomic`. The other pages still need more (examples, etc.).

@steveklabnik

Adding links for Atomics docs rust-lang#29377 r? @steveklabnik This should be good for `std::sync::atomic`. The other pages still need more (examples, etc.).

Rollup of 6 pull requests - Successful merges: #40703, #40728, #40763, #40871, #40935, #40947 - Failed merges:

Adding links for Atomics docs #29377

756f224

rust-highfive assigned steveklabnik Mar 28, 2017

steveklabnik suggested changes Mar 28, 2017

View reviewed changes

Fixing formatting issues #29377

4ea03c8

steveklabnik approved these changes Mar 31, 2017

View reviewed changes

frewsxcv mentioned this pull request Mar 31, 2017

Rollup of 7 pull requests #40969

Closed

frewsxcv mentioned this pull request Mar 31, 2017

Rollup of 6 pull requests #40972

Merged

bors added a commit that referenced this pull request Mar 31, 2017

Auto merge of #40972 - frewsxcv:rollup, r=frewsxcv

2f16528

Rollup of 6 pull requests - Successful merges: #40703, #40728, #40763, #40871, #40935, #40947 - Failed merges:

bors merged commit 4ea03c8 into rust-lang:master Apr 1, 2017

projektir deleted the atomic_links branch April 1, 2017 04:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding links for Atomics docs #29377 #40871

Adding links for Atomics docs #29377 #40871

projektir commented Mar 28, 2017 •

edited

Loading

rust-highfive commented Mar 28, 2017

steveklabnik left a comment

steveklabnik Mar 28, 2017

projektir Mar 28, 2017

steveklabnik Mar 28, 2017

steveklabnik Mar 28, 2017

steveklabnik Mar 28, 2017

steveklabnik Mar 28, 2017

steveklabnik Mar 28, 2017

steveklabnik Mar 28, 2017

steveklabnik Mar 28, 2017

steveklabnik Mar 28, 2017

steveklabnik Mar 28, 2017

steveklabnik Mar 28, 2017

projektir commented Mar 29, 2017

steveklabnik left a comment

steveklabnik commented Mar 31, 2017

bors commented Mar 31, 2017

		/// on the same value.
		/// [1]: http://llvm.org/docs/Atomics.html#acquire

		/// in other threads.
		/// [1]: http://llvm.org/docs/Atomics.html#release

Adding links for Atomics docs #29377 #40871

Adding links for Atomics docs #29377 #40871

Conversation

projektir commented Mar 28, 2017 • edited Loading

rust-highfive commented Mar 28, 2017

steveklabnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

projektir commented Mar 29, 2017

steveklabnik left a comment

Choose a reason for hiding this comment

steveklabnik commented Mar 31, 2017

bors commented Mar 31, 2017

projektir commented Mar 28, 2017 •

edited

Loading