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

Adjust doc comment of Condvar::wait_while #129614

Merged
merged 1 commit into from
Sep 7, 2024
Merged

Adjust doc comment of Condvar::wait_while #129614

merged 1 commit into from
Sep 7, 2024

Conversation

rawler
Copy link
Contributor

@rawler rawler commented Aug 26, 2024

The existing phrasing implies that a notification must be received for wait_while to return. The phrasing is changed to make no such implication.

@rustbot
Copy link
Collaborator

rustbot commented Aug 26, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tgross35 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 26, 2024
@rawler
Copy link
Contributor Author

rawler commented Aug 26, 2024

Proposing this change, since a colleague interpreted the docs as I first did; This function should only be called after first checking condition myself, since it will wait for a notification.

I suspected that interpretation to be wrong however; I've hardly ever seen condvar:s waited upon before an initial condition-check, so this interpretation sounded surprising. Checking the source confirmed my suspicion.

I'm not at all wed to my proposed phrasing, feel free to look for better ways to express it.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@tgross35
Copy link
Contributor

tgross35 commented Sep 4, 2024

Thanks for the updates here! The docs for CondVar are a bit slim all around and don't really show a case where you would actually want to use one, so improvements are definitely welcome.

@rawler rawler force-pushed the patch-1 branch 2 times, most recently from 64eb061 to f01a8fc Compare September 5, 2024 16:17
@rawler
Adjust doc comment of Condvar::wait_while
96837dc 
The existing phrasing implies that a notification must be received for `wait_while` to return. The phrasing is changed to better reflect the behavior.
@tgross35
Copy link
Contributor

tgross35 commented Sep 6, 2024

Sorry this took so many iterations for a pretty minimal fix, but thanks for taking care of the updates!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 6, 2024

📌 Commit 96837dc has been approved by tgross35

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 6, 2024
@compiler-errors compiler-errors mentioned this pull request Sep 7, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 7, 2024
@bors
Auto merge of rust-lang#130066 - compiler-errors:rollup-sq6l0va, r=co…
07776a9 
…mpiler-errors

Rollup of 9 pull requests

Successful merges:

 - rust-lang#128871 (bypass linker configuration and cross target check for specific commands)
 - rust-lang#129468 ([testsuite][cleanup] Remove all usages of `dont_merge` hack to avoid function merging)
 - rust-lang#129614 (Adjust doc comment of Condvar::wait_while)
 - rust-lang#129840 (Implement suggestions for `elided_named_lifetimes`)
 - rust-lang#129891 (Do not request sanitizers for naked functions)
 - rust-lang#129899 (Add Suggestions for Misspelled Keywords)
 - rust-lang#129940 (s390x: Fix a regression related to backchain feature)
 - rust-lang#129987 (Don't store region in `CapturedPlace`)
 - rust-lang#130054 (Add missing quotation marks)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 7, 2024
@bors
Auto merge of rust-lang#130066 - compiler-errors:rollup-sq6l0va, r=co…
4ff16a9 
…mpiler-errors

Rollup of 9 pull requests

Successful merges:

 - rust-lang#128871 (bypass linker configuration and cross target check for specific commands)
 - rust-lang#129468 ([testsuite][cleanup] Remove all usages of `dont_merge` hack to avoid function merging)
 - rust-lang#129614 (Adjust doc comment of Condvar::wait_while)
 - rust-lang#129840 (Implement suggestions for `elided_named_lifetimes`)
 - rust-lang#129891 (Do not request sanitizers for naked functions)
 - rust-lang#129899 (Add Suggestions for Misspelled Keywords)
 - rust-lang#129940 (s390x: Fix a regression related to backchain feature)
 - rust-lang#129987 (Don't store region in `CapturedPlace`)
 - rust-lang#130054 (Add missing quotation marks)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 040be55 into rust-lang:master Sep 7, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 7, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 7, 2024
@rust-timer
Unrolled build for rust-lang#129614
c88526d 
Rollup merge of rust-lang#129614 - rawler:patch-1, r=tgross35

Adjust doc comment of Condvar::wait_while

The existing phrasing implies that a notification must be received for `wait_while` to return. The phrasing is changed to make no such implication.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgross35 tgross35 tgross35 left review comments

Assignees

@tgross35 tgross35

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.83.0
Development

Successfully merging this pull request may close these issues.
5 participants
@rawler @rustbot @rust-log-analyzer @tgross35 @bors