Skip to content

std: Add impl of FnOnce to AssertRecoverSafe #32102

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

Merged
merged 1 commit into from
Mar 11, 2016

Conversation

alexcrichton
Copy link
Member

This was originally intended, but forgot to land by accident!

cc #27719

@alexcrichton
Copy link
Member Author

r? @aturon

cc @sfackler, @wycats, @nikomatsakis

/// // ...
/// ```
///
/// Sometimes this behavior may be a bit too coarse grained, however, and
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment is kind of vague. I would maybe say something like:

Wrapping the entire closure amounts to a blanket assertion that all captured variables are recover safe. This has the downside that if new captures are added in the future, they will also be considered recover safe. Therefore, you may prefer to just wrap individual captures, as shown below. This is more annotation, but it ensures that if a new capture is added which is not recover safe, you will get a compilation error at that time, which will allow you to consider whether that new capture in fact represent a bug or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Updated with this text.

This was originally intended, but forgot to land by accident!

cc rust-lang#27719
@alexcrichton alexcrichton force-pushed the assert-safe-closures branch from 5353be5 to ec58f40 Compare March 7, 2016 19:02
@glaebhoerl
Copy link
Contributor

Is there a reason not to have Fn and FnMut too?

@sfackler
Copy link
Member

sfackler commented Mar 7, 2016

FnOnce is a supertrait of Fn and FnMut, so those come along for free.

@aturon
Copy link
Member

aturon commented Mar 8, 2016

@bors: r+

@bors
Copy link
Collaborator

bors commented Mar 8, 2016

📌 Commit ec58f40 has been approved by aturon

@Stebalien
Copy link
Contributor

@sfackler

FnOnce is a supertrait of Fn and FnMut,

yes

so those come along for free.

no.

Fn implies FnMut implies FnOnce, not the other way arround. If you only implement FnOnce, you can only call call AssertRecoverSafe once.

@aturon
Copy link
Member

aturon commented Mar 8, 2016

We could certainly add Fn and FnMut impls, but there's not much reason to: recover takes a FnOnce.

@glaebhoerl
Copy link
Contributor

Yeah I hadn't absorbed that the only place you'd want this is on the argument to recover. (I suppose other HOFs bounding their arguments by RecoverSafe isn't likely.)

@bors
Copy link
Collaborator

bors commented Mar 10, 2016

⌛ Testing commit ec58f40 with merge 3412421...

@bors
Copy link
Collaborator

bors commented Mar 10, 2016

💔 Test failed - auto-mac-64-opt

@alexcrichton
Copy link
Member Author

@bors: retry

On Thu, Mar 10, 2016 at 11:56 AM, bors [email protected] wrote:

[image: 💔] Test failed - auto-mac-64-opt
http://buildbot.rust-lang.org/builders/auto-mac-64-opt/builds/8326


Reply to this email directly or view it on GitHub
#32102 (comment).

@bors
Copy link
Collaborator

bors commented Mar 11, 2016

⌛ Testing commit ec58f40 with merge c6a6053...

bors added a commit that referenced this pull request Mar 11, 2016
std: Add impl of FnOnce to AssertRecoverSafe

This was originally intended, but forgot to land by accident!

cc #27719
@bors bors merged commit ec58f40 into rust-lang:master Mar 11, 2016
@alexcrichton alexcrichton deleted the assert-safe-closures branch March 11, 2016 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants