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.

Sign up for GitHub

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

Jump to bottom

Spin loop pause function redux #41207

Merged
merged 1 commit into from
May 2, 2017
Merged

Spin loop pause function redux #41207

merged 1 commit into from
May 2, 2017

Conversation

mstewartgallus
Copy link
Contributor

GitHub's interface is screwy.

This is the same PR as #40537

@rust-highfive
Copy link
Collaborator

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

@BurntSushi
Copy link
Member

@alexcrichton What do you think? I saw you raised some concerns here, but I think they've been addressed in this PR.

@BurntSushi BurntSushi added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 11, 2017
@alexcrichton
Copy link
Member

Yes the docs seems reasonable, although I'm not a huge fan of the name hint_core_should_pause. I don't have many suggestions, though. Is there precedent in, say, C++ for this?

@mstewartgallus
Copy link
Contributor Author

@alexcrichton There is no precedent in C or C++ but there is in Java see http://openjdk.java.net/jeps/285 .

@alexcrichton
Copy link
Member

Thanks for the pointer! Looks like Java called this java.lang.Thread.onSpinWait (FWIW)

Not sure if that means much for us...

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 14, 2017
@clarfonthey
Copy link
Contributor

The spin crate calls this function cpu_relax; maybe we should do something similar?

@arielb1
Copy link
Contributor

arielb1 commented Apr 18, 2017 •
edited
Loading

r? @alexcrichton

@alexcrichton is on vacation, so he might not look on your PR until next week.

@arielb1 arielb1 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 18, 2017
@arielb1 arielb1 assigned alexcrichton and unassigned BurntSushi Apr 18, 2017
@bors
Copy link
Contributor

bors commented Apr 20, 2017

☔ The latest upstream changes (presumably #41411) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

Ok this seems like a reasonable API to me to have in libstd, and we've probably had enough bikeshedding at this point! The name seems totally reasonable to me in terms of it's still unstable and we'll have a final pass before stabilizing regardless.

@sstewartgallus if you'd like to rebase I'll r+!

@alexcrichton alexcrichton added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 20, 2017
@bors
Copy link
Contributor

bors commented Apr 23, 2017

☔ The latest upstream changes (presumably #41437) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

Ah sorry looks like I miseed that rebase before bors caught it, wanna try again?

@alexcrichton
Copy link
Member

@sstewartgallus looks like tidy is failing, and mind squashing this into one commit?

mzji
mzji reviewed May 1, 2017
View reviewed changes
src/doc/unstable-book/src/hint-core-should-pause.md Outdated
@@ -0,0 +1,41 @@
# `hint_core_should_pause`
Copy link

@mzji mzji May 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should be moved to library-features directory.

mzji
mzji reviewed May 1, 2017
View reviewed changes
src/doc/unstable-book/src/SUMMARY.md Outdated
@@ -221,3 +222,4 @@
- [windows_handle](library-features/windows-handle.md)
- [windows_net](library-features/windows-net.md)
- [windows_stdio](library-features/windows-stdio.md)
- [zero_one](library-features/zero-one.md)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't zero_one feature anymore, is this a typo?

mzji
mzji reviewed May 1, 2017
View reviewed changes
src/doc/unstable-book/src/SUMMARY.md Outdated
@@ -145,6 +145,7 @@
- [future_atomic_orderings](library-features/future-atomic-orderings.md)
- [get_type_id](library-features/get-type-id.md)
- [heap_api](library-features/heap-api.md)
- [hint_core_should_pause](hint-core-should-pause.md)
Copy link

@mzji mzji May 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the link to point to library-features/hint-core-should-pause.md .

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented May 2, 2017

📌 Commit f4fe3cd has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented May 2, 2017

⌛ Testing commit f4fe3cd with merge 6a5fc9e...

bors added a commit that referenced this pull request May 2, 2017
@bors
Auto merge of #41207 - sstewartgallus:spinpause, r=alexcrichton
6a5fc9e 
Spin loop pause function redux

GitHub's interface is screwy.

This is the same PR as #40537
@bors
Copy link
Contributor

bors commented May 2, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 6a5fc9e to master...

@bors bors merged commit f4fe3cd into rust-lang:master May 2, 2017
@clarfonthey clarfonthey mentioned this pull request May 2, 2017
Closed
@mstewartgallus mstewartgallus deleted the spinpause branch May 2, 2017 21:45
@dtolnay dtolnay mentioned this pull request Nov 8, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mzji mzji mzji left review comments

Assignees

@alexcrichton alexcrichton

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@mstewartgallus @rust-highfive @BurntSushi @alexcrichton @clarfonthey @arielb1 @bors @mzji @Mark-Simulacrum