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 example to std::process::abort #40904

Merged
merged 4 commits into from
Mar 31, 2017
Merged

Add example to std::process::abort #40904

merged 4 commits into from
Mar 31, 2017

Conversation

rap2hpoutre
Copy link
Contributor

This is a first step in order to complete this issue: #29370
I submitted this PR with the help of @steveklabnik More info here: #29370 (comment)

It's my first PR on Rust, I'm learning how to contribute: Should I ping someone? I will post another PR with a more complicated example soon, I prefer send it separately (cause maybe I made some mistakes).

@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 @brson (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.

@steveklabnik
Copy link
Member

Should I ping someone?

Nope, as you can see, @rust-highfive has your back 😄 that said, let's assign this to me, not brian:

r? @steveklabnik

I will post another PR with a more complicated example soon, I prefer send it separately (cause maybe I made some mistakes).

💯

@rust-highfive rust-highfive assigned steveklabnik and unassigned brson Mar 29, 2017
@steveklabnik steveklabnik mentioned this pull request Mar 29, 2017
Closed
14 tasks
steveklabnik
steveklabnik suggested changes Mar 29, 2017
View reviewed changes
Copy link
Member

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

one tiny thing, and then this is good to go! 🎊

src/libstd/process.rs Outdated
@@ -1056,6 +1056,19 @@ pub fn exit(code: i32) -> ! {
/// will be run. If a clean shutdown is needed it is recommended to only call
/// this function at a known point where there are no more destructors left
/// to run.
///
/// # Examples
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

could you add another /// above this line, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for all the time you spent to help me!

Copy link
Member

Choose a reason for hiding this comment

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

No problem! Very glad to do so.

@steveklabnik
Copy link
Member

Looks great, thanks! Travis found an issue though:

https://travis-ci.org/rust-lang/rust/jobs/216364459#L564-L566

these three lines have trailing whitespace. Can you remove them?

@rap2hpoutre
Copy link
Contributor Author

rap2hpoutre commented Mar 29, 2017 •
edited
Loading

Fixed. Thanks again!

@steveklabnik Travis fails again but I don't understand why what I have done make it crash. Reading the log does not help me: https://api.travis-ci.org/jobs/216373325/log.txt?deansi=true

It ends with:

$ cat obj/tmp/sccache.log
cat: obj/tmp/sccache.log: No such file or directory

$ cat /tmp/sccache.log
cat: /tmp/sccache.log: No such file or directory

$ ls $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory

$ dmesg | grep -i kill
[   14.352873] init: failsafe main process (1136) killed by TERM signal


Done. Your build exited with 1.

@steveklabnik
Copy link
Member

Looks like a spurious failure; I'm going to retry it.

ollie27
ollie27 reviewed Mar 30, 2017
View reviewed changes
src/libstd/process.rs Outdated
///
/// # Examples
///
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be changed to ```no_run so it doesn't try to run this code as a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok thanks, done!

@rap2hpoutre
Copy link
Contributor Author

@steveklabnik It passes now :)

@steveklabnik
Copy link
Member

@bors: r+ rollup

wonderful, thank you so much!

@bors
Copy link
Contributor

bors commented Mar 30, 2017

📌 Commit 1be84ce has been approved by steveklabnik

@rap2hpoutre
Copy link
Contributor Author

Thanks to you I learned a lot with this (really small) PR! I'm ready for better PR now.

@steveklabnik
Copy link
Member

steveklabnik commented Mar 30, 2017 via email

@frewsxcv frewsxcv mentioned this pull request Mar 30, 2017
Closed
@frewsxcv frewsxcv mentioned this pull request Mar 30, 2017
Closed
@frewsxcv frewsxcv mentioned this pull request Mar 31, 2017
Closed
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 31, 2017
@frewsxcv
Rollup merge of rust-lang#40904 - rap2hpoutre:patch-1, r=steveklabnik
fe34818 
Add example to std::process::abort

This is a first step in order to complete this issue: rust-lang#29370
I submitted this PR with the help of @steveklabnik More info here: rust-lang#29370 (comment)

It's my first PR on Rust, I'm learning how to contribute: Should I ping someone? I will post another PR with a more complicated example soon, I prefer send it separately (cause maybe I made some mistakes).
@frewsxcv frewsxcv mentioned this pull request Mar 31, 2017
Merged
bors added a commit that referenced this pull request Mar 31, 2017
@bors
Auto merge of #40950 - frewsxcv:rollup, r=frewsxcv
40feadb 
Rollup of 10 pull requests

- Successful merges: #40694, #40842, #40869, #40888, #40898, #40904, #40925, #40928, #40929, #40934
- Failed merges:
@bors bors merged commit 1be84ce into rust-lang:master Mar 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steveklabnik steveklabnik steveklabnik approved these changes

@ollie27 ollie27 ollie27 left review comments

Assignees

@steveklabnik steveklabnik

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rap2hpoutre @rust-highfive @steveklabnik @bors @ollie27 @brson