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

Added tcgetpgrp and tcsetpgrp #451

Merged
merged 6 commits into from
Nov 8, 2016
Merged

Added tcgetpgrp and tcsetpgrp #451

merged 6 commits into from
Nov 8, 2016

Conversation

tinywombat765
Copy link
Contributor

I added the tcgetpgrp and tcsetpgrp functions. I'm not sure if I did everything right so please tell me if I need to change anything.

@posborne
Copy link
Member

Thank you! The implementation looks good to me. Can you do two minor things:

  1. Add documentation to the functions following the basic style present in the changes from this PR (which should be merged soon): Documentation improvements #449
  2. Please add a note about the new functions to the CHANGELOG.md file in the root of the project.

@tinywombat765
Copy link
Contributor Author

Sure. I'll get in that.

@homu
Copy link
Contributor

homu commented Oct 29, 2016

☔ The latest upstream changes (presumably #449) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Member

@kamalmarhubi kamalmarhubi left a comment

Choose a reason for hiding this comment

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

This seems fine to me. One small change to tcsetgrp, and then @posborne's request for a changelog entry and docs. If the example docs are to much, just a short one-line would still be useful.

#[inline]
pub fn tcsetpgrp(fd: c_int, pgrp: pid_t) -> Result<c_int> {
let res = unsafe { libc::tcsetpgrp(fd, pgrp) };
Errno::result(res)
Copy link
Member

Choose a reason for hiding this comment

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

this should have .map(drop), as the return only distinguishes failures.

@tinywombat765
Copy link
Contributor Author

I've made the request changes.

@kamalmarhubi
Copy link
Member

Thanks @zethra!

@homu r+

@homu
Copy link
Contributor

homu commented Nov 8, 2016

📌 Commit 013e7c8 has been approved by kamalmarhubi

@homu
Copy link
Contributor

homu commented Nov 8, 2016

⌛ Testing commit 013e7c8 with merge 1dc37a0...

homu added a commit that referenced this pull request Nov 8, 2016
Added tcgetpgrp and tcsetpgrp

I added the tcgetpgrp and tcsetpgrp functions.  I'm not sure if I did everything right so please tell me if I need to change anything.
@homu
Copy link
Contributor

homu commented Nov 8, 2016

💥 Test timed out

@kamalmarhubi
Copy link
Member

@homu retry

@homu
Copy link
Contributor

homu commented Nov 8, 2016

⌛ Testing commit 013e7c8 with merge bf8ce97...

homu added a commit that referenced this pull request Nov 8, 2016
Added tcgetpgrp and tcsetpgrp

I added the tcgetpgrp and tcsetpgrp functions.  I'm not sure if I did everything right so please tell me if I need to change anything.
@homu
Copy link
Contributor

homu commented Nov 8, 2016

☀️ Test successful - status

@homu homu merged commit 013e7c8 into nix-rust:master Nov 8, 2016
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.

4 participants