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.

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 a Result::into_ok_or_err method to extract a T from Result<T, T> #80572

Merged
merged 4 commits into from
Feb 18, 2021

Conversation

thomcc
Copy link
Member

@thomcc thomcc commented Jan 1, 2021

When updating code to handle the semi-recent deprecation of compare_and_swap in favor of compare_exchange, which returns Result<T, T>, I wanted this. I've also wanted it with code using slice::binary_search before.

The name (and perhaps the documentation) is the hardest part here, but this name seems consistent with the other Result methods, and equivalently memorable.

@rust-highfive
Copy link
Collaborator

r? @shepmaster

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 1, 2021
library/core/src/result.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Jan 1, 2021

ok_or_err makes me think of ok_or(), something like fn ok_or_err(self, err: E) -> Self. Maybe into_inner() is a better name?

@jyn514 jyn514 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-result-option Area: Result and Option combinators labels Jan 1, 2021
@thomcc
Copy link
Member Author

thomcc commented Jan 1, 2021

I'm comfortable renaming if desired. I think ok_or_err is clearer than into_inner, but into_inner is pretty good too (better than most of the other suggestions I got, which were: merge, collapse, converge, either_value, unwrap_either, ...).

I don't care that much about the name so long as the functionality is available under some name though.

Another suggestion I got was implementing From<Result<T, T>> for (bool, T), which is probably possible, but I didn't like, mainly because that seems strictly less convenient. For example, in the cas=>cmpxchg transition, I'd need to make atom.compare_and_swap(a, b, <ordering>) into <(bool, T)>::from(atom.compare_exchange(a, b, <ordering0>, <ordering1>)).1, which is awkward.

/// cases where you don't care if the result was `Ok` or not.
///
/// [`Atomic*::compare_exchange`]: crate::sync::atomic::AtomicBool::compare_exchange
/// [binary_search]: ../../std/primitive.slice.html#method.binary_search
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that:

That said, I'm also okay with getting rid of these links entirely if that's desirable.

Copy link
Member

Choose a reason for hiding this comment

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

You can now use intra-doc links here!

Suggested change
/// [binary_search]: ../../std/primitive.slice.html#method.binary_search
/// [binary_search]: slice::binary_search

Copy link
Member

Choose a reason for hiding this comment

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

Actually, better yet you can change the part above that has the link to just [`slice::binary_search`] and remove this line.

Copy link
Member

Choose a reason for hiding this comment

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

You can now use intra-doc links here!

No, the bootstrap bump hasn't happened yet. Intra-doc pointers landed in #80181, which is in 1.51, which is still in nightly.

Copy link
Member

Choose a reason for hiding this comment

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

Dang, I always forget about the bootstrap bump :(

@shepmaster
Copy link
Member

Maybe into_inner() is a better name?

This at least tracks with the usage in futures (and either).

/// `Err`.
///
/// This can be useful in conjunction with APIs such as
/// [`Atomic*::compare_exchange`], or [`slice::binary_search`][binary_search], but only in
Copy link
Member

Choose a reason for hiding this comment

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

Binary search is the primary reason I've wanted similar, but I've also seen people want to use it in procedural macros where you use a TokenStream for success and failure cases.

This seems reasonable to add as an unstable feature.

@vandenheuvel
Copy link
Contributor

Naming consistency with into_ok seems logical, so I propose a name like into_*, such as into_either or into_ok_or_err.

@camelid camelid added the S-waiting-on-bikeshed Status: Awaiting a decision on trivial things. label Jan 22, 2021
@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 24, 2021
@bors
Copy link
Contributor

bors commented Jan 26, 2021

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

@m-ou-se
Copy link
Member

m-ou-se commented Feb 17, 2021

r? @m-ou-se

@rust-highfive rust-highfive assigned m-ou-se and unassigned shepmaster Feb 17, 2021
@m-ou-se m-ou-se added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). S-waiting-on-bikeshed Status: Awaiting a decision on trivial things. labels Feb 17, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Feb 17, 2021

We briefly discussed this in the libs team meeting last week, and agreed this is a function we should have. There was some discussion on the name, but felt comfortable landing this as ok_or_err or into_ok_or_err and leaving the bikeshedding for later (before stabilization).

Can you open a tracking issue? Thanks!

@thomcc
Copy link
Member Author

thomcc commented Feb 17, 2021

Result::into_ok existing (even as unstable) is a compelling reason to use into_ok_or_err, so I renamed it (for now, I guess).

@thomcc thomcc changed the title Add a Result::ok_or_err method to extract a T from Result<T, T> Add a ~~Result::ok_or_err~~ Result::into_ok_or_err method to extract a T from Result<T, T> Feb 17, 2021
@thomcc thomcc changed the title Add a ~~Result::ok_or_err~~ Result::into_ok_or_err method to extract a T from Result<T, T> Add a Result::into_ok_or_err method to extract a T from Result<T, T> Feb 17, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Feb 17, 2021

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 17, 2021

📌 Commit 404da0b has been approved by m-ou-se

@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 Feb 17, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#77728 (Expose force_quotes on Windows.)
 - rust-lang#80572 (Add a `Result::into_ok_or_err` method to extract a `T` from `Result<T, T>`)
 - rust-lang#81860 (Fix SourceMap::start_point)
 - rust-lang#81869 (Simplify pattern grammar, improve or-pattern diagnostics)
 - rust-lang#81898 (Fix debug information for function arguments of type &str or slice.)
 - rust-lang#81972 (Placeholder lifetime error cleanup)
 - rust-lang#82007 (Implement reborrow for closure captures)
 - rust-lang#82021 (Spell out nested Self type in lint message)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 40e3af5 into rust-lang:master Feb 18, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-result-option Area: Result and Option combinators S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.