Use core::time::Duration with TimeDelta as output type only

[esheppa]

Setting this as draft while I figure out a good way to split up the commit into manageable units

[djc]

Great work! Some thoughts/suggestions from an initial round of skimming.

[esheppa]

will move f6dceba into the proper commits once I've got the CI passing

[djc]

I think the constituent pieces here are looking good. In terms of the commit history, I think instead of leaving the removal of all code related to the OldTimeDelta until the end, each commit that adds implementations for Duration should also remove the similar implementations for OldTimeDelta. That way it's much easier to review if we're adding back equivalent operations.

You also added DaysDelta and MonthsDelta. It seems like these are additive (that is, they don't exactly replace equivalent functionaly that would be removed with OldTimeDelta)? If they are not additive, it would be good to demonstrate that using the same approach (removing code in the same commit as these are added).

[djc]

Nit: first line of the docstring should fit on one line.

[esheppa]

should be resolved in the latest force push

[djc]

Nit: shorter first docstring line.

[esheppa]

should be resolved in the latest force push

[djc]

Where's the precedent for this name? What does overflowing mean here? Conceptually it seems like checked_ and overflowing_ would usually be mutually exclusive.

[esheppa]

I've renamed to overflowing_add_opt but perhaps there is a better option.

The original intention was to use checked because of the Option and overflowing represents the fact that it is ok for the Duration to be longer than the time to the end of the day.

[djc]

Why is this related to the TimeDelta work?

[esheppa]

This used to rely on the fact that the duration could potentially be negative or positive, now that it can't and so the situation is a bit more complex and the signed integers are harder to work with.

An option that works which is closer to the original implementation is:

let mut secs = i64::from(self.secs) - i64::from(rhs.secs);
let mut frac = i64::from(self.frac) - i64::from(rhs.frac);

// `secs` may contain a leap second yet to be counted
let adjust = match self.secs.cmp(&rhs.secs) {
    Ordering::Greater => {
        if rhs.frac >= 1_000_000_000 {
            1
        } else {
            0
        }
    }
    Ordering::Equal => 0,
    Ordering::Less => {
        if self.frac >= 1_000_000_000 {
            -1
        } else {
            0
        }
    }
};

secs += adjust;

while frac < 0 {
    frac += 1_000_000_000;
    secs -= 1;
}
match secs.cmp(&0) {
    Ordering::Equal => {
        TimeDelta::Forwards(Duration::new(0, u32::try_from(frac).unwrap()))
    }
    Ordering::Greater => {
        TimeDelta::Forwards(Duration::new(u64::try_from(secs).unwrap(), u32::try_from(frac).unwrap()))
    }
    Ordering::Less => {
        TimeDelta::Backwards(Duration::new(u64::try_from(secs.abs()).unwrap(), u32::try_from(frac.abs()).unwrap()))
    }
}

let me know which you think is better? One benefit of the other way is that it doesn't have any unwraps

[djc]

Don't have time to review this in detail today. One question on my mind: is there some way we can start exposing parts of this change on 0.4.x already? Presumably we could just add the TimeDelta type and add methods that return it and/or Add/Sub impls that take std::time::Duration, right?

[esheppa]

can start exposing parts of this change on 0.4.x

This should be quite possible. Almost all the methods have new names as well. The PR is actually more problematic in main because we have already used the TimeDelta name. The new code without the removals could be quite cleanly applied in 0.4.x I think.

As an additional thought, what do you think about deprecating the Add / Sub impls - these don't expose the fallibility anyway (although arguably the fallibility there is primarily overflow which is already tolerated in Add/Sub impls), and then this would make the methods the primary API, meaning we can expose essentially all of this in 0.4.x.

Do you think this would be better structured as a PR to 0.4.x adding the new code, and a PR to main which removes the old code?

[djc]

I think it would be great to first do a PR to 0.4.x which adds new API and deprecates old stuff (I forget if/how well deprecations on impl blocks work?) merge that to main and then clean up the old stuff.

Good point about Add/Sub not exposing fallibility anyway.

[pitdicker]

Personally I don't think an enum wrapping an u64 to pretend to be an signed integer is a good direction. Especially if we are not even using most of the range of that u64. std::time::Duration is a little more than an u64, but the principle is close.

I'm closing this PR for now because it hasn't seen any activity in the last year while it is going to need a lot of work to become merge-able.