-
Notifications
You must be signed in to change notification settings - Fork 654
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
Add alarm
and cancel_alarm
#828
Conversation
src/unistd.rs
Outdated
/// a second will be ignored (not rounded up). E.g. 1 second and 800 miliseconds | ||
/// will become 1 second. | ||
/// | ||
/// Also, Strictly conforming the POSIX spec, duriations longer then `65535` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO the warning in the POSIX specification is dumb. It just warns you against using constants greater than UINT_MAX
, which is something your compiler can already warn you about. However, this Rust wrapper introduces a new problem: secs
is a u64
, which can overflow at runtime. You should probably assert
that secs <= libc::c_uint::max_value()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want just an assert, or a silent convertion into the max value with a warning in the doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think an assert would be better than a saturating conversion. After all, why would somebody ever set an alarm for more than 4 billion seconds into the future? Such a thing is probably a bug.
src/unistd.rs
Outdated
/// if any is canceld. | ||
/// | ||
/// [`alarm`]: fn.alarm.html | ||
pub fn cancel_alarm() -> Option<Duration> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this function really necessary? Can't we accomplish the same thing just by making alarm
's argument an Option<Duration>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will change it.
test/test_unistd.rs
Outdated
assert_eq!(alarm(Duration::from_secs(1)), Some(Duration::from_secs(60))); | ||
let start = Instant::now(); | ||
|
||
let handler = SigHandler::Handler(alarm_signal_handler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should set up the hander before calling alarm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, however this test takes at least a second so I though I add the handler after so it removes some of the overhead, but I'll before it before the call.
I addressed all feedback and squashed the commits. |
test/test_unistd.rs
Outdated
|
||
#[test] | ||
fn test_canceling_alarm() { | ||
#[allow(unused_variables)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you can eliminate this directive if you change the variable's name from m
to _m
. That didn't work with older Rust releases, but it should be possible now that the minimum Rust is 1.20.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall I do it for all uses while I'm at it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do, make it a separate commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I'll make a seperate PR for that, including this line.
test/test_unistd.rs
Outdated
static mut ALARM_CALLED: bool = false; | ||
|
||
// Used in `test_alarm`. | ||
#[no_mangle] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of no_mangle
? Using extern
should be sufficient to pass the function pointer to a C function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copied a handle from my code, which (if I can remeber correctly) needed this. Will see if I can remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right it wasn't needed.
I'm not a big fan of using |
@Susurrus I don't really mind either way, I just though to make the API little more Rust-like. |
Chages the parameter and return value to |
The tests are failing because Buildbot can't figure out how to clone your repo. Probably something to do with your most recent force push. But it looks like you need to rebase anyway, to resolve the conflicts. Could you please do that? |
This should be good now, trying to resolve the conflict on the GitHub website messed up the branch for me. But I reset based on the new master and it should be good. |
Looking at this diff on GitHub shows it as adding all of |
I'll make another pr tomorrow from a fresh repo, although I'm not sure what when wrong. |
New pr in #830. |
No description provided.