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

Improve docs for Waker::noop and LocalWaker::noop #128064

Merged
merged 3 commits into from
Aug 16, 2024
Merged

Improve docs for Waker::noop and LocalWaker::noop #128064

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b18c7d8
Docs for Waker and LocalWaker: Add cross-refs in comment
ijackson Jul 22, 2024
c404406
LocalWaker docs: Make long-ago omitted but probably intended changes
ijackson Jul 22, 2024
9a95573
Add cautionary paragraph about noop wakers.
ijackson Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion library/core/src/task/wake.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,10 +531,18 @@ impl Waker {

/// Returns a reference to a `Waker` that does nothing when used.
///
// Note! Much of the documentation for this method is duplicated
// in the docs for `LocalWaker::noop`.
// If you edit it, consider editing the other copy too.
//
/// This is mostly useful for writing tests that need a [`Context`] to poll
/// some futures, but are not expecting those futures to wake the waker or
/// do not need to do anything specific if it happens.
///
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would read slightly better as a single paragraph, since the "More generally…" is directly referring to what was just introduced, whereas the paragraphs above and below this pair are much more unrelated.

Suggested change
///

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I prefer it in two paragraphs. The first paragraph is about tests; then the second paragraph is indeed more general.

I think if we join these together the order should be reversed, so that the general case, with the clear statement about discarded notifications, isn't buried in a paragraph which on first glance seems to be just about testing.

/// More generally, using `Waker::noop()` to poll a future
/// means discarding the notification of when the future should be polled again.
/// So it should only be used when such a notification will not be needed to make progress.
///
/// If an owned `Waker` is needed, `clone()` this one.
///
/// # Examples
Expand Down Expand Up @@ -782,12 +790,22 @@ impl LocalWaker {
Self { waker }
}

/// Creates a new `LocalWaker` that does nothing when `wake` is called.
/// Returns a reference to a `LocalWaker` that does nothing when used.
///
// Note! Much of the documentation for this method is duplicated
// in the docs for `Waker::noop`.
// If you edit it, consider editing the other copy too.
//
Comment on lines +795 to +798
Copy link
Member

Choose a reason for hiding this comment

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

This is pervasively true, I don't feel it's worth a comment? If you want to fix it, then I would prefer a macro that expands into a few doc attrs, so it's actually fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it's pervasively true. I have seen similar situations in many other places in the rust-lang/rust codebase, often with unintended divergence as we see here. I'm afraid I don't have the tuits to engage in a campaign of docs deduplication. I find it difficult to predict the style and taste preferences in the Rust Project. I feel someone closer to the core of the project should demonstrate how this ought to be done, and probably the libs team should (at least implicitly, by lazy consensus) approve the idiom. (The docs are not precisely identical, so the macro wouldn't be entirely straightforward.)

IMO the comment is very helpful, even with the remaining duplication, because it tells you precisely where you need to edit the clone-and-hack. If that comment had been in place, perhaps 6f8a944 wouldn't have contained the slip. It's fairly lightweight answer to the problem.

If you don't agree, or want to insist that I do substantially more work instead of this stopgap, I will remove these hunks from this MR branch.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... I see. I tried to do the refactoring I originally suggested and found it more of a pain in the ass than I expected. As-is, this seems adequate. It makes me vaguely annoyed, which made me initially reluctant about it, but on reflection it is an annoyance with the preexisting state of things.

/// This is mostly useful for writing tests that need a [`Context`] to poll
/// some futures, but are not expecting those futures to wake the waker or
/// do not need to do anything specific if it happens.
///
Copy link
Contributor

Choose a reason for hiding this comment

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

(Matching above suggestion.)

Suggested change
///

/// More generally, using `LocalWaker::noop()` to poll a future
/// means discarding the notification of when the future should be polled again,
/// So it should only be used when such a notification will not be needed to make progress.
///
/// If an owned `LocalWaker` is needed, `clone()` this one.
///
/// # Examples
///
/// ```
Expand Down
Loading