-
Notifications
You must be signed in to change notification settings - Fork 222
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
No overflow panic on very long timeouts #111
Conversation
src/condvar.rs
Outdated
#[cfg(feature = "nightly")] | ||
let very_long_timeout = Duration::from_secs(u64::max_value()); | ||
#[cfg(not(feature = "nightly"))] | ||
let very_long_timeout = Duration::from_millis(u32::max_value() as u64); |
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.
This would look better with if cfg!(feature = "nightly") {} else {}
.
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 instinctively always try to use #[cfg]
to make the unwanted code go away as early as possible in the compile :)
I also think that won't work. Since checked_add
does not exist on non-nightly. So it does not compile. if false { checked_add() }
still requires checked_add
even if it's never going to be executed.
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.
Sorry.. I misread which file the comment was on. Yes, here we can use cfg!
. Fixed it.
The Linux CI error seems to be because Some of the windows errors seems to be because So maybe we need to bump the minimum version here anyway? Or lock these dependencies to very specific older patch releases. Requiring a newer compiler sounds very much like a breaking change to me, so I don't understand why these crates didn't bump the major version, but anyway. |
fd53e62
to
ad5dc16
Compare
ad5dc16
to
e593f10
Compare
Unstable support for checked_add on time types was finally merged (rust-lang/rust#56490) so now we can avoid the possible panic that is
Instant::now() + timeout
.First commit is unrelated. It's just some other simplification I thought it was nice to make.