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

Add #[must_use] message to Iterator and Future #57549

Merged
merged 3 commits into from Jan 13, 2019

Conversation

Projects
None yet
6 participants
@taiki-e
Copy link
Contributor

taiki-e commented Jan 12, 2019

Iterator's message is based on current iterator adaptor's #[must_use] message (added in #15561) and https://github.com/rust-lang/rust/pull/56677/files#r241236020
Future's message is the same as those used in futures-rs and tokio.

r? @Centril

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 12, 2019

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

@@ -88,7 +88,7 @@ fn _assert_is_object_safe(_: &dyn Iterator<Item=()>) {}
message="`{Self}` is not an iterator"
)]
#[doc(spotlight)]
#[must_use]
#[must_use = "iterator adaptors are lazy and do nothing unless consumed"]

This comment has been minimized.

@euclio

euclio Jan 12, 2019

Contributor

The existing docs use "iterator adapter", so I think we want to use that spelling instead.

This comment has been minimized.

@taiki-e

taiki-e Jan 12, 2019

Author Contributor

It seems that "iterator adaptors" is used as #[must_use] message: https://github.com/rust-lang/rust/blob/master/src/libcore/iter/mod.rs#L407

This comment has been minimized.

@euclio

euclio Jan 12, 2019

Contributor

Hm, that one should probably be changed too, then.

This comment has been minimized.

@taiki-e

taiki-e Jan 12, 2019

Author Contributor

In the Rust code on GitHub, iterator adaptors seems to be used more than iterator adapter.

IMO, this should probably be handled by another PR that like "Unify the name of the iterator adapter".

This comment has been minimized.

@Centril

Centril Jan 12, 2019

Contributor

My preference is to just drop "adaptors" in favor of "iterators are lazy and do...", since it's more direct. It seems preferable to unify everything, in whichever direction we move, in this PR.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 13, 2019

I believe you missed:

//! warning: unused result that must be used: iterator adaptors are lazy and
//! do nothing unless consumed

In iter/mod.rs


Since this now drops "adaptors" from the message as I suggested I'd like to get a second opinion on this change. (It is true that not all iterators are necessarily lazy since constructing them can perform work, but that's a level of detail that I think is missing the forest for the trees...)

r? @estebank

@rust-highfive rust-highfive assigned estebank and unassigned Centril Jan 13, 2019

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Jan 13, 2019

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 13, 2019

📌 Commit da933cc has been approved by estebank

Centril added a commit to Centril/rust that referenced this pull request Jan 13, 2019

Rollup merge of rust-lang#57549 - taiki-e:must_use, r=estebank
Add #[must_use] message to Iterator and Future

~~Iterator's message is based on current iterator adaptor's #[must_use] message (added in rust-lang#15561) and https://github.com/rust-lang/rust/pull/56677/files#r241236020~~
Future's message is the same as those used in [futures-rs](https://github.com/rust-lang-nursery/futures-rs/search?q=must_use&unscoped_q=must_use) and [tokio](https://github.com/tokio-rs/tokio/search?q=must_use&unscoped_q=must_use).

r? @Centril

Centril added a commit to Centril/rust that referenced this pull request Jan 13, 2019

Rollup merge of rust-lang#57549 - taiki-e:must_use, r=estebank
Add #[must_use] message to Iterator and Future

~~Iterator's message is based on current iterator adaptor's #[must_use] message (added in rust-lang#15561) and https://github.com/rust-lang/rust/pull/56677/files#r241236020~~
Future's message is the same as those used in [futures-rs](https://github.com/rust-lang-nursery/futures-rs/search?q=must_use&unscoped_q=must_use) and [tokio](https://github.com/tokio-rs/tokio/search?q=must_use&unscoped_q=must_use).

r? @Centril

Centril added a commit to Centril/rust that referenced this pull request Jan 13, 2019

Rollup merge of rust-lang#57549 - taiki-e:must_use, r=estebank
Add #[must_use] message to Iterator and Future

~~Iterator's message is based on current iterator adaptor's #[must_use] message (added in rust-lang#15561) and https://github.com/rust-lang/rust/pull/56677/files#r241236020~~
Future's message is the same as those used in [futures-rs](https://github.com/rust-lang-nursery/futures-rs/search?q=must_use&unscoped_q=must_use) and [tokio](https://github.com/tokio-rs/tokio/search?q=must_use&unscoped_q=must_use).

r? @Centril

Centril added a commit to Centril/rust that referenced this pull request Jan 13, 2019

Rollup merge of rust-lang#57549 - taiki-e:must_use, r=estebank
Add #[must_use] message to Iterator and Future

~~Iterator's message is based on current iterator adaptor's #[must_use] message (added in rust-lang#15561) and https://github.com/rust-lang/rust/pull/56677/files#r241236020~~
Future's message is the same as those used in [futures-rs](https://github.com/rust-lang-nursery/futures-rs/search?q=must_use&unscoped_q=must_use) and [tokio](https://github.com/tokio-rs/tokio/search?q=must_use&unscoped_q=must_use).

r? @Centril

bors added a commit that referenced this pull request Jan 13, 2019

Auto merge of #57577 - Centril:rollup, r=Centril
Rollup of 4 pull requests

Successful merges:

 - #57004 (Make `TokenStream` less recursive.)
 - #57102 (NLL: Add union justifications to conflicting borrows.)
 - #57337 (rustc: Place wasm linker args first instead of last)
 - #57549 (Add #[must_use] message to Iterator and Future)

Failed merges:

r? @ghost

@bors bors merged commit da933cc into rust-lang:master Jan 13, 2019

@taiki-e taiki-e deleted the taiki-e:must_use branch Jan 14, 2019

kennytm added a commit to kennytm/rust that referenced this pull request Feb 8, 2019

Rollup merge of rust-lang#58262 - taiki-e:must_use, r=estebank
Add #[must_use] message to Fn* traits

This PR adds `#[must_use]` message to `Fn*` traits.

Related: rust-lang#57549

r? @estebank

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Feb 10, 2019

Rollup merge of rust-lang#58262 - taiki-e:must_use, r=estebank
Add #[must_use] message to Fn* traits

This PR adds `#[must_use]` message to `Fn*` traits.

Related: rust-lang#57549

r? @estebank

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Feb 10, 2019

Rollup merge of rust-lang#58262 - taiki-e:must_use, r=estebank
Add #[must_use] message to Fn* traits

This PR adds `#[must_use]` message to `Fn*` traits.

Related: rust-lang#57549

r? @estebank

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Feb 10, 2019

Rollup merge of rust-lang#58262 - taiki-e:must_use, r=estebank
Add #[must_use] message to Fn* traits

This PR adds `#[must_use]` message to `Fn*` traits.

Related: rust-lang#57549

r? @estebank

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Feb 10, 2019

Rollup merge of rust-lang#58262 - taiki-e:must_use, r=estebank
Add #[must_use] message to Fn* traits

This PR adds `#[must_use]` message to `Fn*` traits.

Related: rust-lang#57549

r? @estebank
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment