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

Use a different representation for Duration #16466

Closed
brson opened this issue Aug 12, 2014 · 1 comment · Fixed by #16626
Closed

Use a different representation for Duration #16466

brson opened this issue Aug 12, 2014 · 1 comment · Fixed by #16626

Comments

@brson
Copy link
Contributor

brson commented Aug 12, 2014

The Duration type introduced in #15934 looks like

pub struct Duration {                 
    days: i32,                                                                
    secs: u32,  // Always < SECS_PER_DAY                                     
    nanos: u32, // Always < NANOS_PR_SECOND                                                   
}

Per @kballard's [comment] this could be

pub struct Duration {                                                                 
    secs: i64,       
    nanos: u32, // Always < NANOS_PR_SECOND                                                   
}
@ruuda
Copy link
Contributor

ruuda commented Aug 19, 2014

I'd like to work on this.

ruuda added a commit to ruuda/rust that referenced this issue Aug 20, 2014
This changes the internal representation of `Duration` from

    days: i32,
    secs: i32,
    nanos: u32

to

    secs: i64,
    nanos: i32

This resolves rust-lang#16466. Some methods now take `i64` instead of `i32` due
to the increased range. Some methods, like `num_milliseconds`, now
return an `Option<i64>` instead of `i64`, because the range of
`Duration` is now larger than e.g. 2^63 milliseconds.
liigo added a commit to liigo/rust that referenced this issue Aug 21, 2014
…type `i64`

Closes rust-lang#16466.

The return type of `to_tuple_64`, `num_milliseconds` etc. has been change too, so this change is

[breaking-change]
bors added a commit that referenced this issue Aug 28, 2014
This changes the internal representation of `Duration` from

    days: i32,
    secs: i32,
    nanos: u32

to

    secs: i64,
    nanos: i32

This resolves #16466. Note that `nanos` is an `i32` and not `u32` as suggested, because `i32` is easier to deal with, and it is not exposed anyway. Some methods now take `i64` instead of `i32` due to the increased range. Some methods, like `num_milliseconds`, now return an `Option<i64>` instead of `i64`, because the range of `Duration` is now larger than e.g. 2^63 milliseconds.

A few remarks:
- Negating `MIN` is impossible. I chose to return `MAX` as `-MIN`, but it is one nanosecond less than the actual negation. Is this the desired behaviour?
- In `std::io::timer`, some functions accept a `Duration`, which is internally converted into a number of milliseconds. However, the range of `Duration` is now larger than 2^64 milliseconds. There is already a FIXME in the file that this should be addressed (without a ticket number though). I chose to silently use 0 ms if the duration is too long. Is that right, as long as the backend still uses milliseconds?
- Negative durations are not formatted correctly, but they were not formatted correctly before either.
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 a pull request may close this issue.

2 participants