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

No overflow panic on very long timeouts #111

Merged
merged 5 commits into from
Dec 15, 2018

Conversation

faern
Copy link
Collaborator

@faern faern commented Dec 15, 2018

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.

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);
Copy link
Owner

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 {}.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

@faern
Copy link
Collaborator Author

faern commented Dec 15, 2018

The Linux CI error seems to be because rustc_demangle started using the rename dependency feature in the last release, coming from this commit: rust-lang/rustc-demangle@d74ae4b#diff-80398c5faae3c069e4e6aa2ed11b28c0

Some of the windows errors seems to be because backtrace started using the #[repr(align(x))] feature.

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.

@Amanieu Amanieu merged commit fd4af45 into Amanieu:master Dec 15, 2018
@faern faern deleted the no-instant-overflow-panic branch December 15, 2018 16:18
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

2 participants