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

Make Stdio handle UnwindSafe #51973

Merged
merged 1 commit into from
Jul 3, 2018
Merged

Make Stdio handle UnwindSafe #51973

merged 1 commit into from
Jul 3, 2018

Conversation

estk
Copy link
Contributor

@estk estk commented Jul 1, 2018

Closes #51863

This is my first compiler PR. Thanks Niko for the mentor help!

r? @nikomatsakis

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 1, 2018
}
#[test]
fn stdoutlock_unwind_safe() {
assert_unwind_safe::<StdoutLock<'static>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to pass in the explicit lifetime here.

@nikomatsakis
Copy link
Contributor

This is my first compiler PR. Thanks Niko for the mentor help!

Glad to hear it! I'm not sure that it was me who helped you, though =) maybe @abonander ?

@nikomatsakis
Copy link
Contributor

@bors delegate=abonander =)

@bors
Copy link
Contributor

bors commented Jul 2, 2018

✌️ @abonander can now approve this pull request

@estk
Copy link
Contributor Author

estk commented Jul 2, 2018

Whoops, yep for sure it was @abonander. Thanks to you @abonander. 😄

Copy link
Contributor

@abonander abonander left a comment

Choose a reason for hiding this comment

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

If this builds it's probably going to infer a single lifetime whereas I would like to somehow notate a requirement that the trait be implemented for all lifetimes. I'll test locally and see if for<'a> StdoutLock<' a> works.

@estk
Copy link
Contributor Author

estk commented Jul 2, 2018

@abonander interesting, not a feature I knew even existed. It compiled locally and I figured it was fine since the lifetime param seemed irrelevant to whether the StdoutLock and hence ReentrantMutex implemented UnwindSafe and RefUnwindSafe.

Wrt your last comment, StdoutLock is a struct so does not appear to be compatible with HRTBs.

Where do we go from here? Drop the lifetime or pin to 'static?

@abonander
Copy link
Contributor

Lifetimes may not be relevant now but I'm sure one could contrive a few ways to break it without breaking the test as it is written.

My use-case involves 'static (via lazy_static!()) so I would selfishly like a regression test specifically covering that case. Probably just add that to what you have and we'll call it good.

@abonander
Copy link
Contributor

And if you don't mind, would you squash the later commits into the first one to keep the history clean?

@abonander
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 2, 2018

📌 Commit 9797665 has been approved by abonander

@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 Jul 2, 2018
@abonander
Copy link
Contributor

Excellent!

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Jul 3, 2018
Make Stdio handle UnwindSafe

Closes  rust-lang#51863

This is my first compiler PR. Thanks Niko for the mentor help!

r? @nikomatsakis
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Jul 3, 2018
Make Stdio handle UnwindSafe

Closes  rust-lang#51863

This is my first compiler PR. Thanks Niko for the mentor help!

r? @nikomatsakis
bors added a commit that referenced this pull request Jul 3, 2018
Rollup of 13 pull requests

Successful merges:

 - #51548 (Initialize LLVM's AMDGPU target machine, if available.)
 - #51809 (Add read_exact_at and write_all_at methods to FileExt on unix)
 - #51914 (add outlives annotations to `BTreeMap`)
 - #51958 (Show known meta items in unknown meta items error)
 - #51973 (Make Stdio handle UnwindSafe)
 - #51977 (bootstrap: tests should use rustc from config.toml)
 - #51978 (Do not suggest changes to str literal if it isn't one)
 - #51979 (Get rid of `TyImplTraitExistential`)
 - #51980 (Emit column info in debuginfo for non msvc like targets)
 - #51982 (incr.comp.: Take names of children into account when computing the ICH of a module's HIR.)
 - #51997 (add entry for cargo-metadata feature to RELEASES)
 - #52004 (toolstate: Fixed detection of changed submodule, and other fixes.)
 - #52006 ( Change --keep-stage to apply more often)

Failed merges:

r? @ghost
@bors bors merged commit 9797665 into rust-lang:master Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants