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

Lower ? to Try instead of Carrier #42275

Merged
merged 3 commits into from
Jun 1, 2017
Merged

Lower ? to Try instead of Carrier #42275

merged 3 commits into from
Jun 1, 2017

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented May 28, 2017

The easy parts of rust-lang/rfcs#1859, whose FCP completed without further comments.

Just the trait and the lowering -- neither the error message improvements nor the insta-stable impl for Option nor exhaustive docs.

Based on a github search, this will break the following:

The other results appear to be files from libcore or its tests. I could also leave Carrier around after stage0 and impl<T:Carrier> Try for T if that would be better.

r? @nikomatsakis

Edit: Oh, and it might accidentally improve perf, based on #37939 (comment), since Try::into_result for Result is an obvious no-op, unlike Carrier::translate.

The easy parts of RFC 1859.  (Just the trait and the lowering, none of
the error message improvements nor the insta-stable impl for Option.)
@@ -0,0 +1,7 @@
# `try_trait`

The tracking issue for this feature is: [#31436]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we say a bit more here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a bit more unstable-book text. I'm uncertain exactly what's supposed to be in it; is there anything in particular you'd like me to address there?

/// in terms of a success/failure dichotomy. This trait allows both
/// extracting those success or failure values from an existing instance and
/// creating a new instance from a success or failure value.
#[unstable(feature = "try_trait", issue = "31436")]
Copy link
Contributor

Choose a reason for hiding this comment

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

so I also re-used #31436 as the tracking issue, but I wonder if we should make a new issue just for this feature? It would seem to make things a bit clearer e.g. when we call for FCP...

Copy link
Contributor

Choose a reason for hiding this comment

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

@nikomatsakis As policy I tend to favor making tracking issues as fine-grained as possible, specifically because of the nightmare that the original ? RFC became due to needless conflation with catch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Created #42327 and updated the attributes.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

This looks good as a start! The only thing that may be worth doing is adding a bit more text into the "unstable book".

@nikomatsakis
Copy link
Contributor

cc @pfpacket @peterdelevoryas -- just a heads-up that, when this lands, the (unstable) Carrier trait is going to be replaced with Try.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 31, 2017

📌 Commit 3119e63 has been approved by nikomatsakis

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Jun 1, 2017
Lower `?` to `Try` instead of `Carrier`

The easy parts of rust-lang/rfcs#1859, whose FCP completed without further comments.

Just the trait and the lowering -- neither the error message improvements nor the insta-stable impl for Option nor exhaustive docs.

Based on a [github search](https://github.com/search?l=rust&p=1&q=question_mark_carrier&type=Code&utf8=%E2%9C%93), this will break the following:

- https://github.com/pfpacket/rust-9p/blob/00206e34c680198a0ac7c2f066cc2954187d4fac/src/serialize.rs#L38
- https://github.com/peterdelevoryas/bufparse/blob/b1325898f4fc2c67658049196c12da82548af350/src/result.rs#L50

The other results appear to be files from libcore or its tests.  I could also leave Carrier around after stage0 and `impl<T:Carrier> Try for T` if that would be better.

r? @nikomatsakis

Edit: Oh, and it might accidentally improve perf, based on rust-lang#37939 (comment), since `Try::into_result` for `Result` is an obvious no-op, unlike `Carrier::translate`.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Jun 1, 2017
Lower `?` to `Try` instead of `Carrier`

The easy parts of rust-lang/rfcs#1859, whose FCP completed without further comments.

Just the trait and the lowering -- neither the error message improvements nor the insta-stable impl for Option nor exhaustive docs.

Based on a [github search](https://github.com/search?l=rust&p=1&q=question_mark_carrier&type=Code&utf8=%E2%9C%93), this will break the following:

- https://github.com/pfpacket/rust-9p/blob/00206e34c680198a0ac7c2f066cc2954187d4fac/src/serialize.rs#L38
- https://github.com/peterdelevoryas/bufparse/blob/b1325898f4fc2c67658049196c12da82548af350/src/result.rs#L50

The other results appear to be files from libcore or its tests.  I could also leave Carrier around after stage0 and `impl<T:Carrier> Try for T` if that would be better.

r? @nikomatsakis

Edit: Oh, and it might accidentally improve perf, based on rust-lang#37939 (comment), since `Try::into_result` for `Result` is an obvious no-op, unlike `Carrier::translate`.
bors added a commit that referenced this pull request Jun 1, 2017
Rollup of 9 pull requests

- Successful merges: #42136, #42275, #42286, #42297, #42302, #42306, #42314, #42324, #42347
- Failed merges:
@bors bors merged commit 3119e63 into rust-lang:master Jun 1, 2017
@scottmcm scottmcm deleted the try-trait branch June 1, 2017 07:53
pfpacket added a commit to pfpacket/rust-9p that referenced this pull request Jun 2, 2017
Direct replacement of std::op::Carrier
rust-lang/rust#42275
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants