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

stabilized compiler_fences (fixes #41091) #44595

Merged
merged 3 commits into from Sep 17, 2017

Conversation

Projects
None yet
8 participants
@budziq
Copy link
Contributor

budziq commented Sep 15, 2017

I did not know what to proceed with "unstable-book" entry. The feature would no longer be unstable so I have deleted it. If it was the wrong call I'll revert it (unfortunately his case is not described in the CONTRIBUTING.md).

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Sep 15, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (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.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 15, 2017

Thanks! Maybe the docs could move to the function?

@budziq

This comment has been minimized.

Copy link
Contributor Author

budziq commented Sep 15, 2017

Maybe the docs could move to the function?

I've translated part of the unstable-book article into an example. I'm not sure if it didn't came out forced.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 15, 2017

@bors: r+

Sure!

cc @jonhoo

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 15, 2017

📌 Commit 571941b has been approved by alexcrichton

@jonhoo

This comment has been minimized.

Copy link
Contributor

jonhoo commented Sep 15, 2017

Hmm, I think I would argue for more of the text from the unstable book to be incorporated into the documentation for compiler_fence. This is a very specialized function, and having the docs to explain that is probably a good idea. In particular, I think the last paragraph before the Examples header, and the first paragraph after, could be kept verbatim in the docs for compiler_fence, perhaps under a heading of "Usage"? There's also the question about whether it is appropriate to refer readers to the equivalent function in C++ like the unstable book text does, though I feel less strongly about that.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 15, 2017

@bors: r-

@budziq want to update with @jonhoo's thoughts?

@budziq

This comment has been minimized.

Copy link
Contributor Author

budziq commented Sep 15, 2017

want to update with @jonhoo's thoughts?

@alexcrichton Gladly! Although I might not have time to approach it this weekend.

In particular, I think the last paragraph before the Examples header, and the first paragraph after, could be kept verbatim

@jonhoo I've originally omitted these paragraphs as these convey the same message as current docs present above compiler_fence (although in much terser form). I can replace one with the other.

There's also the question about whether it is appropriate to refer readers to the equivalent function in C++

I'm not sure about linking to C++ reference but I would prefer not to point to StackOverflow, leaving only the linux kernel link.

@budziq budziq force-pushed the budziq:stabilize_compiler_fences branch from 571941b to 5f62c0c Sep 16, 2017

@budziq

This comment has been minimized.

Copy link
Contributor Author

budziq commented Sep 16, 2017

I've updated the docs with the best I could come up with :)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 16, 2017

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 16, 2017

📌 Commit 5f62c0c has been approved by alexcrichton

@jonhoo

This comment has been minimized.

Copy link
Contributor

jonhoo commented Sep 16, 2017

LGTM 👍

TimNN added a commit to TimNN/rust that referenced this pull request Sep 17, 2017

Rollup merge of rust-lang#44595 - budziq:stabilize_compiler_fences, r…
…=alexcrichton

stabilized compiler_fences (fixes rust-lang#41091)

I did not know what to proceed with "unstable-book" entry. The feature would no longer be unstable so I have deleted it. If it was the wrong call I'll revert it (unfortunately his case is not described in the CONTRIBUTING.md).

@TimNN TimNN referenced this pull request Sep 17, 2017

Closed

Rollup of 19 pull requests #44649

bors added a commit that referenced this pull request Sep 17, 2017

Auto merge of #44649 - TimNN:rollup, r=TimNN
Rollup of 19 pull requests

- Successful merges: #44073, #44088, #44381, #44397, #44509, #44515, #44533, #44549, #44553, #44562, #44567, #44573, #44595, #44604, #44617, #44622, #44630, #44639, #44647
- Failed merges:

TimNN added a commit to TimNN/rust that referenced this pull request Sep 17, 2017

Rollup merge of rust-lang#44595 - budziq:stabilize_compiler_fences, r…
…=alexcrichton

stabilized compiler_fences (fixes rust-lang#41091)

I did not know what to proceed with "unstable-book" entry. The feature would no longer be unstable so I have deleted it. If it was the wrong call I'll revert it (unfortunately his case is not described in the CONTRIBUTING.md).

@TimNN TimNN referenced this pull request Sep 17, 2017

Closed

Rollup of 18 pull requests #44652

bors added a commit that referenced this pull request Sep 17, 2017

Auto merge of #44652 - TimNN:rollup, r=TimNN
Rollup of 18 pull requests

- Successful merges: #44073, #44088, #44381, #44397, #44509, #44515, #44533, #44549, #44553, #44562, #44567, #44595, #44604, #44617, #44622, #44630, #44639, #44647
- Failed merges:

TimNN added a commit to TimNN/rust that referenced this pull request Sep 17, 2017

Rollup merge of rust-lang#44595 - budziq:stabilize_compiler_fences, r…
…=alexcrichton

stabilized compiler_fences (fixes rust-lang#41091)

I did not know what to proceed with "unstable-book" entry. The feature would no longer be unstable so I have deleted it. If it was the wrong call I'll revert it (unfortunately his case is not described in the CONTRIBUTING.md).

@TimNN TimNN referenced this pull request Sep 17, 2017

Merged

Rollup of 17 pull requests #44654

bors added a commit that referenced this pull request Sep 17, 2017

Auto merge of #44654 - TimNN:rollup, r=TimNN
Rollup of 17 pull requests

- Successful merges: #44073, #44088, #44381, #44397, #44509, #44533, #44549, #44553, #44562, #44567, #44595, #44604, #44617, #44622, #44630, #44639, #44647
- Failed merges:

@bors bors merged commit 5f62c0c into rust-lang:master Sep 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@budziq budziq deleted the budziq:stabilize_compiler_fences branch Sep 17, 2017

The way it is currently written, the `assert_eq!` is *not* guaranteed to
succeed, despite everything happening in a single thread. To see why,
remember that the compiler is free to swap the stores to
`IMPORTANT_VARIABLE` and `IS_READ` since they are both

This comment has been minimized.

@nalply

nalply Sep 21, 2017

Perhaps a typo? IS_READY instead of IS_READ (perhaps occurs several times)

This comment has been minimized.

@jonhoo

jonhoo Sep 21, 2017

Contributor

Yes, this should be IS_READY

dtolnay added a commit to dtolnay/rust that referenced this pull request Sep 25, 2017

Backport libs stabilizations to 1.21 beta
This includes the following stabilizations:

- tcpstream_connect_timeout rust-lang#44563
- iterator_for_each rust-lang#44567
- ord_max_min rust-lang#44593
- compiler_fences rust-lang#44595
- needs_drop rust-lang#44639
- vec_splice rust-lang#44640

dtolnay added a commit to dtolnay/rust that referenced this pull request Sep 25, 2017

Backport libs stabilizations to 1.21 beta
This includes the following stabilizations:

- tcpstream_connect_timeout rust-lang#44563
- iterator_for_each rust-lang#44567
- ord_max_min rust-lang#44593
- compiler_fences rust-lang#44595
- needs_drop rust-lang#44639
- vec_splice rust-lang#44640

dtolnay added a commit to dtolnay/rust that referenced this pull request Sep 26, 2017

Backport libs stabilizations to 1.21 beta
This includes the following stabilizations:

- tcpstream_connect_timeout rust-lang#44563
- iterator_for_each rust-lang#44567
- ord_max_min rust-lang#44593
- compiler_fences rust-lang#44595
- needs_drop rust-lang#44639
- vec_splice rust-lang#44640

bors added a commit that referenced this pull request Sep 26, 2017

Auto merge of #44823 - dtolnay:backport21, r=alexcrichton
[beta] Backport accepted PRs to 1.21

Backport of:

- ~don't suggest placing `use` statements into expanded code #44215
- stabilize tcpstream_connect_timeout #44563
- stabilized iterator_for_each #44567
- travis: Move sccache to the us-west-1 region #44574
- stabilized ord_max_min #44593
- stabilized compiler_fences #44595
- ci: Upload/download from a new S3 bucket #44617
- stabilized needs_drop #44639
- Stabilized vec_splice and modified splice tracking issue #44640
- Backport libs stabilizations to 1.21 beta #44824

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Sep 29, 2017

Rollup merge of rust-lang#44824 - dtolnay:22really21, r=alexcrichton
Backport libs stabilizations to 1.21 beta

Includes the following stabilizations:

- tcpstream_connect_timeout rust-lang#44563
- iterator_for_each rust-lang#44567
- ord_max_min rust-lang#44593
- compiler_fences rust-lang#44595
- needs_drop rust-lang#44639
- vec_splice rust-lang#44640

These have been backported in rust-lang#44823.

@dtolnay dtolnay referenced this pull request Oct 10, 2017

Merged

1.21 announcement #203

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.