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

Jump to bottom

Add the try_trait_v2 library basics #84092

Merged
merged 2 commits into from
Apr 27, 2021
Merged

Add the try_trait_v2 library basics #84092

merged 2 commits into from
Apr 27, 2021

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Apr 11, 2021 •
edited
Loading

No compiler changes as part of this -- just new unstable traits and impls thereof.

The goal here is to add the things that aren't going to break anything, to keep the feature implementation simpler in the next PR.

(Draft since the FCP won't end until Saturday, but I was feeling optimistic today -- and had forgotten that FCP was 10 days, not 7 days.)

@rust-highfive
Copy link
Collaborator

r? @kennytm

(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 Apr 11, 2021
@kennytm kennytm added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 11, 2021
@scottmcm scottmcm mentioned this pull request Apr 17, 2021
Open
23 tasks
@scottmcm
Add the try_trait_v2 library basics
1864970 
No compiler changes as part of this -- just new unstable traits and impls thereof.

The goal here is to add the things that aren't going to break anything, to keep the feature implementation simpler in the next PR.
@scottmcm scottmcm force-pushed the try_trait_initial branch from 7b32024 to 1864970 Compare April 17, 2021 18:58
@scottmcm scottmcm marked this pull request as ready for review April 17, 2021 19:41
@scottmcm
Copy link
Member Author

Updated with a real tracking issue, now that the RFC is merged, so I think this is good to go now.

(Sorry for not ghosting it previously.)

@scottmcm scottmcm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. F-try_trait_v2 Tracking issue for RFC#3058 and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Apr 17, 2021
yaahc
yaahc reviewed Apr 20, 2021
View reviewed changes
Copy link
Member

@yaahc yaahc left a comment

Choose a reason for hiding this comment

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

Very exciting!

library/core/src/ops/control_flow.rs Show resolved Hide resolved
library/core/src/ops/mod.rs Show resolved Hide resolved
library/core/src/ops/try_trait.rs Outdated Show resolved Hide resolved
library/core/src/ops/try_trait.rs Show resolved Hide resolved
library/core/src/ops/try_trait.rs Outdated Show resolved Hide resolved
library/core/src/ops/try_trait.rs
Comment on lines +93 to +95
/// But this "call `branch`, then `match` on it, and `return` if it was a
/// `Break`" is exactly what happens inside the `?` operator. So rather than
/// do all this manually, we can just use `?` instead:
Copy link
Member

Choose a reason for hiding this comment

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

Oooo this is soo satisfying.

library/core/src/ops/try_trait.rs Outdated Show resolved Hide resolved
library/core/src/ops/try_trait.rs Outdated Show resolved Hide resolved
library/core/src/ops/try_trait.rs Outdated Show resolved Hide resolved
@Johannesd3

This comment has been minimized.

Sign in to view
@scottmcm

This comment has been minimized.

Sign in to view
@scottmcm scottmcm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 20, 2021
@scottmcm scottmcm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 25, 2021
@yaahc
Copy link
Member

yaahc commented Apr 26, 2021 •
edited
Loading

This PR looks good to me. I would like to see a bullet point on further planned documentation updates recorded somewhere, presumably on the tracking issue.

@scottmcm
Copy link
Member Author

It had the standard "adjust documentation" checkbox, but that's often more about the reference and such, so I've added another one for more library documentation in #84277.

@yaahc
Copy link
Member

yaahc commented Apr 26, 2021

Since the RFC has merged am I right in assuming this is ready to merge now?

@yaahc
Copy link
Member

yaahc commented Apr 26, 2021

yolo

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Apr 26, 2021 •
edited by yaahc
Loading

Team member @yaahc has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 26, 2021
@scottmcm
Copy link
Member Author

scottmcm commented Apr 26, 2021 •
edited
Loading

As this is all unstable I don't think it needs an FCP? It should be entirely invisible to stable code.

(Shouldn't impact any unstable code either, since it's just additions. The next PR that will change the bounds on iterator methods and such might need more eyes on it, but the goal for this PR was to not have to.)

@m-ou-se
Copy link
Member

m-ou-se commented Apr 26, 2021

Looks great to me!

Also verified that this only adds unstable things and has no effect on existing code.

@bors r=yaahc,m-ou-se

@bors
Copy link
Contributor

bors commented Apr 26, 2021

📌 Commit 5671647 has been approved by yaahc,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 Apr 26, 2021
@bors
Copy link
Contributor

bors commented Apr 26, 2021

⌛ Testing commit 5671647 with merge 61e1715...

@bors
Copy link
Contributor

bors commented Apr 27, 2021

☀️ Test successful - checks-actions
Approved by: yaahc,m-ou-se
Pushing 61e1715 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 27, 2021
@bors bors merged commit 61e1715 into rust-lang:master Apr 27, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 27, 2021
@scottmcm scottmcm deleted the try_trait_initial branch April 27, 2021 02:36
@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 27, 2021
@scottmcm scottmcm mentioned this pull request May 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yaahc yaahc yaahc left review comments

Assignees

@kennytm kennytm

Labels
F-try_trait_v2 Tracking issue for RFC#3058 merged-by-bors This PR was explicitly merged by bors. 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
Milestone
1.53.0
Development

Successfully merging this pull request may close these issues.
9 participants
@scottmcm @rust-highfive @Johannesd3 @yaahc @rfcbot @m-ou-se @bors @kennytm @rustbot