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.

Sign up for GitHub

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

Jump to bottom

Create the subsec_millis and subsec_micros methods for Duration #1351

Closed
wants to merge 3 commits into from
Closed

Create the subsec_millis and subsec_micros methods for Duration #1351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2297696
Create the subsec_millis and subsec_micros methods for Duration
botahamec Nov 4, 2023
2a95dc5
Change the documentation for subsec methods
botahamec Nov 8, 2023
10cd25f
Move constants to `lib.rs`
botahamec Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
71 changes: 52 additions & 19 deletions src/duration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,24 +19,10 @@ use std::error::Error;
#[cfg(feature = "rkyv")]
use rkyv::{Archive, Deserialize, Serialize};

/// The number of nanoseconds in a microsecond.
const NANOS_PER_MICRO: i32 = 1000;
/// The number of nanoseconds in a millisecond.
const NANOS_PER_MILLI: i32 = 1_000_000;
/// The number of nanoseconds in seconds.
const NANOS_PER_SEC: i32 = 1_000_000_000;
/// The number of microseconds per second.
const MICROS_PER_SEC: i64 = 1_000_000;
/// The number of milliseconds per second.
const MILLIS_PER_SEC: i64 = 1000;
/// The number of seconds in a minute.
const SECS_PER_MINUTE: i64 = 60;
/// The number of seconds in an hour.
const SECS_PER_HOUR: i64 = 3600;
/// The number of (non-leap) seconds in days.
const SECS_PER_DAY: i64 = 86_400;
/// The number of (non-leap) seconds in a week.
const SECS_PER_WEEK: i64 = 604_800;
use crate::{
MICROS_PER_SEC, MILLIS_PER_SEC, NANOS_PER_MICRO, NANOS_PER_MILLI, NANOS_PER_SEC, SECS_PER_DAY,
SECS_PER_HOUR, SECS_PER_MINUTE, SECS_PER_WEEK,
};

macro_rules! try_opt {
($e:expr) => {
Expand Down Expand Up @@ -221,7 +207,9 @@ impl Duration {
}
}

/// Returns the number of nanoseconds such that
/// Returns the number of nanoseconds in the fractional part of the duration.
Copy link
Contributor

@djc djc Nov 20, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These improvements for the new methods should be introduced in the same commit as the methods themselves, as I explained in my previous comment. Only the changes for existing methods should remain in this commit.

///
/// This is the number of nanoseconds such that
/// `subsec_nanos() + num_seconds() * NANOS_PER_SEC` is the total number of
/// nanoseconds in the duration.
pub const fn subsec_nanos(&self) -> i32 {
Expand All @@ -232,6 +220,24 @@ impl Duration {
}
}

/// Returns the number of microseconds in the fractional part of the duration.
///
/// This is the number of microseconds such that
/// `subsec_micros() + num_seconds() * MICROS_PER_SEC` is the total number of
botahamec marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pitdicker same question as below regarding pub MICROS_PER_SEC.

/// microseconds in the duration.
pub const fn subsec_micros(&self) -> i32 {
self.subsec_nanos() / NANOS_PER_MICRO
}

/// Returns the number of milliseconds in the fractional part of the duration.
///
/// This is the number of milliseconds such that
/// `subsec_millis() + num_seconds() * MILLIS_PER_SEC` is the total number of
Copy link
Contributor

@jtmoon79 jtmoon79 Nov 4, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this docstring specificity.

@pitdicker should MILLIS_PER_SEC be made pub? If a user wants to follow this docstring implied suggestion (compute subsec_millis() + num_seconds() * MILLIS_PER_SEC) then they'll want to access MILLIS_PER_SEC. This docstring is taken from subsec_nanos docstring which has the same implied suggestion but using NANOS_PER_SEC, so NANOS_PER_SEC should probably also be pub.

Stretch goal would be to make all the constants in that clump of constants above into pub.

Copy link
Contributor

@djc djc Nov 6, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to make these constants part of the public API.

For these docstrings, I'd like to keep a shorter first line, more like core (which has "Returns the fractional part of this Duration, in whole microseconds."). We can still show this formula, but it shouldn't be in the first line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A made a change like that to the docstrings. I'm wondering if those constants should stay in the duration module. They might make more sense if they were put in the lib.rs. I can't imagine anyone looking for these constants intuiting that they'd be in duration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just made that change, but I do have one more question. Now that these are public, I find it a bit strange that some of them are i32 and some are i64. I think this makes sense for what we're using them for, and if it's useful for us to have them that way, then it's probably useful for someone else too, but I wanted to bring it in case someone thinks we should make it more consistent.

Copy link
Contributor

@jtmoon79 jtmoon79 Nov 16, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it a bit strange that some of them are i32 and some are i64

I believe the goal for using i32 is to lessen resource "footprint" chrono objects.

/// milliseconds in the duration.
pub const fn subsec_millis(&self) -> i32 {
self.subsec_nanos() / NANOS_PER_MILLI
}

/// Returns the total number of whole milliseconds in the duration,
pub const fn num_milliseconds(&self) -> i64 {
// A proper Duration will not overflow, because MIN and MAX are defined
Expand Down Expand Up @@ -647,6 +653,33 @@ mod tests {
assert_eq!(Duration::days(-i64::MAX / NANOS_PER_DAY - 1).num_nanoseconds(), None);
}

#[test]
fn test_duration_subsec_nanos() {
assert_eq!(Duration::zero().subsec_nanos(), 0);
assert_eq!(Duration::nanoseconds(1).subsec_nanos(), 1);
assert_eq!(Duration::nanoseconds(-1).subsec_nanos(), -1);
assert_eq!(Duration::seconds(1).subsec_nanos(), 0);
assert_eq!(Duration::nanoseconds(1_000_000_001).subsec_nanos(), 1);
}

#[test]
fn test_duration_subsec_micros() {
assert_eq!(Duration::zero().subsec_micros(), 0);
assert_eq!(Duration::microseconds(1).subsec_micros(), 1);
assert_eq!(Duration::microseconds(-1).subsec_micros(), -1);
assert_eq!(Duration::seconds(1).subsec_micros(), 0);
assert_eq!(Duration::microseconds(1_000_001).subsec_micros(), 1);
}

#[test]
fn test_duration_subsec_millis() {
assert_eq!(Duration::zero().subsec_millis(), 0);
assert_eq!(Duration::milliseconds(1).subsec_millis(), 1);
assert_eq!(Duration::milliseconds(-1).subsec_millis(), -1);
assert_eq!(Duration::seconds(1).subsec_millis(), 0);
assert_eq!(Duration::milliseconds(1_000_001).subsec_millis(), 1);
}

#[test]
fn test_duration_checked_ops() {
assert_eq!(
Expand Down
19 changes: 19 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,25 @@ pub mod rkyv {
pub use crate::weekday::ArchivedWeekday;
}

/// The number of nanoseconds in a microsecond.
pub const NANOS_PER_MICRO: i32 = 1000;
/// The number of nanoseconds in a millisecond.
pub const NANOS_PER_MILLI: i32 = 1_000_000;
/// The number of nanoseconds in a second.
pub const NANOS_PER_SEC: i32 = 1_000_000_000;
/// The number of microseconds in a second.
pub const MICROS_PER_SEC: i64 = 1_000_000;
Copy link
Collaborator

@pitdicker pitdicker Jan 30, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The types of these constants are 'whatever needs the fewest casts in our current code'.

That is: everything that has to do with seconds or more is using the type of the Duration.secs field.
Everything that has to do with subsecond units uses the type of the Duration.nanos field.
For most constants that seems like a good default.

NANOS_PER_SEC and MICROS_PER_SEC are at the boundary. NANOS_PER_SEC is currently an i32, and we sometimes cast it to an i64.

Maybe we should change MICROS_PER_SEC to also be an i32. Argument is consistency with NANOS_PER_SEC, and that widening casts are infallible and available with a From impl (even though this is just a constant and would not fail anyway).

/// The number of milliseconds in a second.
pub const MILLIS_PER_SEC: i64 = 1000;
/// The number of seconds in a minute.
pub const SECS_PER_MINUTE: i64 = 60;
/// The number of seconds in an hour.
pub const SECS_PER_HOUR: i64 = 3600;
/// The number of (non-leap) seconds in a day.
pub const SECS_PER_DAY: i64 = 86_400;
/// The number of (non-leap) seconds in a week.
pub const SECS_PER_WEEK: i64 = 604_800;

/// Out of range error type used in various converting APIs
#[derive(Clone, Copy, Hash, PartialEq, Eq)]
pub struct OutOfRange {
Expand Down