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

Follow-up of #19 and more documentation #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Draphar
Copy link
Contributor

@Draphar Draphar commented May 3, 2019

After the change to as_millis for Windows, I changed it here as well. Because the method is only supported since Rust 1.33, the 1.21 build on Travis is failing. Is that a problem?

I also added a little bit more information about the underlying mechanisms and their limitations as you suggested.

@alexcrichton
Copy link
Owner

Thanks for the PR! I'm not sure that this is quite right though? The intention is that it's valid to pass in any Duration and we'll simply loop until the duration lapses, and if that involves multiple system calls that should be fine (as they're on the order of years apart anyway)

@Draphar
Copy link
Contributor Author

Draphar commented May 7, 2019

Sorry, could you eloborate on the problem? My intention was just to use replace manual conversion to milliseconds with the dedicated method in std. Everything else was supposed to be kept intact.

@alexcrichton
Copy link
Owner

Oh sure, so this is accidentally introducing a behavior change where if a long duration is specified it's truncated and/or panics. The intention is that all durations are valid (there are no max durations) and we just handle it in each implementation.

@Draphar
Copy link
Contributor Author

Draphar commented May 10, 2019

That certainly wasn't the intention. Do you want to keep the documentation?

@alexcrichton
Copy link
Owner

I think the documentation is unfortunately slightly incorrect as well because there's no maximum timeout when calling these methods

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