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

Move SigSet, TimeVal to libc, add TimeSpec and pselect syscall. #276

Closed
wants to merge 4 commits into from

Conversation

abbradar
Copy link
Contributor

Part and extension of #270 and should be merged after #272. Maybe we want to split this across multiple PRs too (although this time these things are dependent on each other).

@abbradar
Copy link
Contributor Author

I guess we should use proper time_t instead of i64 and stuff -- I'll fix this later.

@kamalmarhubi
Copy link
Member

When you do, could you also rebase on top of master now that #272 was merged?

@kamalmarhubi
Copy link
Member

(Or drop the duplicate commits)

@homu
Copy link
Contributor

homu commented Mar 1, 2016

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

@abbradar
Copy link
Contributor Author

abbradar commented Mar 1, 2016

Just to give a signal -- I remember about those and I'll continue working at them when I get more time. Sorry for the delay!

@kamalmarhubi
Copy link
Member

@abbradar no worries! And thanks again for your contributions and interest! :-)

@abbradar
Copy link
Contributor Author

abbradar commented May 7, 2016

Argh, I've just now rebased it atop master and now I cannot find old comments which you've done.... @kamalmarhubi , can you please review this once more time! Or, even better, is there any way to actually get those old comments when you've force-pushed?

@fiveop
Copy link
Contributor

fiveop commented May 7, 2016

I checked my emails. There were no other comments of @kamalmarhubi on this PR.

@abbradar
Copy link
Contributor Author

abbradar commented May 7, 2016

I was talking about his old comments. Said that, I was able to find them in the mailbox archive -- sorry for the noise! I'll go through them shortly.

@abbradar
Copy link
Contributor Author

abbradar commented May 7, 2016

I've moved TimeVal and TimeSpec to the newtype pattern. I think those are the only relevant comments now; if I'm mistaken, please correct me!

assert_eq!(TimeVal::minutes(3) + TimeVal::seconds(2),
TimeVal::seconds(182));
assert!(TimeVal::seconds(1) + TimeVal::seconds(2) == TimeVal::seconds(3));
assert!(TimeVal::minutes(3) + TimeVal::seconds(2) == TimeVal::seconds(182));
Copy link
Member

Choose a reason for hiding this comment

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

assert_eq is preferable because it prints the left and right hand sides

let fd = fd as usize;
self.bits[fd / BITS] & (1 << (fd % BITS)) > 0
pub fn contains(&self, fd: RawFd) -> bool {
assert!(fd >= 0 && fd < FD_SETSIZE, "RawFd out of bounds");
Copy link
Member

Choose a reason for hiding this comment

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

we can probably just return false in this case, while preserving expected meaning?

@kamalmarhubi
Copy link
Member

I made some comments. I do think though that it would be easier to review further changes if each commit was its own PR.

@kamalmarhubi
Copy link
Member

(And thanks for patience, and for coming back to this PR!)

@homu
Copy link
Contributor

homu commented Aug 26, 2016

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

@Susurrus Susurrus added this to the 1.0 milestone Nov 5, 2017
@Susurrus
Copy link
Contributor

@abbradar I think most of these changes have already been done or are not decided on, but the addition of pselect() would be most welcome. Are you still interested in merging that? If you can remove the other 3 commits here, we'd be all set!

@abbradar
Copy link
Contributor Author

I can rebase of course but I don't have much left over from my old project from that time so I cannot test it responsibly now.

@Susurrus
Copy link
Contributor

@abbradar That's fair and I'm going to close this then. Thanks anyways for submitting this PR, sorry it languished so long.

@Susurrus Susurrus closed this Nov 16, 2017
@Susurrus Susurrus removed this from the 1.0 milestone Nov 16, 2017
antifuchs added a commit to antifuchs/nix that referenced this pull request Apr 28, 2018
This is a straight port of @abbradar's work in nix-rust#276, with
two (somewhat weak) tests and a bit of documentation.
@antifuchs antifuchs mentioned this pull request Apr 28, 2018
antifuchs added a commit to antifuchs/nix that referenced this pull request Apr 28, 2018
This is a straight port of @abbradar's work in nix-rust#276, with
two (somewhat weak) tests and a bit of documentation.
antifuchs added a commit to antifuchs/nix that referenced this pull request Apr 28, 2018
This is a straight port of @abbradar's work in nix-rust#276, with
two (somewhat weak) tests and a bit of documentation.
bors bot added a commit that referenced this pull request Apr 28, 2018
894: Add pselect syscall r=asomers a=antifuchs

I saw that #276 was closed, and now I need `pselect`, so here it is! I copied the function body from @abbradar's work, updated the type signatures, added two tests and added a doc comment.

Hope this works!

Co-authored-by: Andreas Fuchs <asf@boinkor.net>
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.

None yet

5 participants