Recommends lazily evaluated alternatives for Option::or and Result::or
#46548
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
the use of lazily evaluated alternatives when appropriate. These comments are intended to echo a clippy lint on the same topic (see https://rust-lang-nursery.github.io/rust-clippy/master/index.html#or_fun_call)
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dtolnay (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. |
kennytm
reviewed
Dec 7, 2017
src/libcore/option.rs
Outdated
| @@ -338,6 +338,10 @@ impl<T> Option<T> { | |||
|
|
|||
| /// Returns the contained value or a default. | |||
| /// | |||
| /// Arguments passed to `unwrap_or` are eagerly evaluated; if you are passing | |||
| /// the result of a function call, it is recommended to use `unwrap_or_else`, | |||
There was a problem hiding this comment.
Could you link to the unwrap_or_else method? i.e.
/// ... it is recommended to use [`unwrap_or_else`],
///
/// ...
///
/// [`unwrap_or_else`]: #method.unwrap_or_else
Similar for the other additions.
kennytm
added
S-waiting-on-review
kennytm
reviewed
Dec 7, 2017
src/libcore/result.rs
Outdated
| @@ -690,6 +694,10 @@ impl<T, E> Result<T, E> { | |||
| /// Unwraps a result, yielding the content of an [`Ok`]. | |||
| /// Else, it returns `optb`. | |||
| /// | |||
| /// Arguments passed to `unwrap_or` are eagerly evaluated; if you are passing | |||
There was a problem hiding this comment.
There is a trailing whitespace on this line, causing the CI to reject this PR. Other places affected:
[00:04:39] tidy error: /checkout/src/libcore/result.rs:697: trailing whitespace
[00:04:39] tidy error: /checkout/src/libcore/result.rs:698: trailing whitespace
[00:04:39] tidy error: /checkout/src/libcore/result.rs:699: trailing whitespace
[00:04:39] tidy error: /checkout/src/libcore/option.rs:341: trailing whitespace
[00:04:39] tidy error: /checkout/src/libcore/option.rs:342: trailing whitespace
[00:04:39] tidy error: /checkout/src/libcore/option.rs:343: trailing whitespace
[00:04:41] some tidy checks failed
dtolnay
approved these changes
Dec 7, 2017
|
@bors r+ rollup |
|
📌 Commit 5847d0b has been approved by |
kennytm
added
S-waiting-on-bors
GuillaumeGomez
added a commit
to GuillaumeGomez/rust
that referenced
this pull request
Dec 7, 2017
Recommends lazily evaluated alternatives for `Option::or` and `Result::or` Adds language to docs for `Option` and `Result` recommending the use of lazily evaluated alternatives when appropriate. These comments are intended to echo a [clippy lint] on the same topic. The [reddit discussion] may also be of interest. [clippy lint]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#or_fun_call [reddit discussion]: https://www.reddit.com/r/rust/comments/7hutqn/perils_of_optionor_and_resultor/
JohnTitor
added a commit
to JohnTitor/rust
that referenced
this pull request
Aug 24, 2022
…JohnTitor Add a warning about `Option/Result::and()` being eagerly evaluated Copied from `or()`. Inspired by [this StackOverflow question](https://stackoverflow.com/questions/73461846/why-is-in-rust-the-expression-in-option-and-evaluated-if-option-is-none). [The PR for `or()`](rust-lang#46548) mentions the Clippy lint `or_fun_call` which doesn't exist for `and()` (although there is `unnecessary_lazy_evaluations`). I still think this warning is also good for `and()`. Feel free to close if you disagree.
workingjubilee
pushed a commit
to tcdi/postgrestd
that referenced
this pull request
Sep 15, 2022
Add a warning about `Option/Result::and()` being eagerly evaluated Copied from `or()`. Inspired by [this StackOverflow question](https://stackoverflow.com/questions/73461846/why-is-in-rust-the-expression-in-option-and-evaluated-if-option-is-none). [The PR for `or()`](rust-lang/rust#46548) mentions the Clippy lint `or_fun_call` which doesn't exist for `and()` (although there is `unnecessary_lazy_evaluations`). I still think this warning is also good for `and()`. Feel free to close if you disagree.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds language to docs for
OptionandResultrecommending the use of lazily evaluated alternatives when appropriate. These comments are intended to echo a clippy lint on the same topic. The reddit discussion may also be of interest.