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

Add Duration type to std and use it in Timer methods. #15934

Closed
wants to merge 22 commits into from

Conversation

brson
Copy link
Contributor

@brson brson commented Jul 24, 2014

Currently, the Timer methods take an integer number of ms. This is considered a bug because a) types, b) some timers have ns precision.

This plucks the Duration type from rust-chrono, plops it into std::time, and replaces the arguments to sleep, oneshot, and periodic timers with it. It leaves the old methods intact as sleep_ms, oneshot_ms, and periodic_ms, for convenience.

Closes #11189.

cc @lifthrasiir @aturon @kballard @alexcrichton

let hasdate = self.days != 0;
let hastime = (self.secs != 0 || self.nanos != 0) || !hasdate;

try!('P'.fmt(f));
Copy link
Member

Choose a reason for hiding this comment

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

Using fmt passes through all format qualifiers like width and such, which probably isn't desired for just the single character P (this may want to be write!)

Copy link
Contributor

Choose a reason for hiding this comment

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

I've fixed this issue on f7065f16254e3e9853e8d82777575ba3b343b952. I didn't think about format specifiers at all, thank you for pointing out that.

@alexcrichton
Copy link
Member

Some other thoughts:

  • Implementation-wise, this is a candidate for residing in libcore. Functionality-wise, I'm not sure if it belongs in libcore.
  • The name "time" implies functionality like getting the current time, but the name "duration" is quite long.
  • I wouldn't expect the duplication of foo_ms, I would expect them all to be deprecated/removed

@alexcrichton
Copy link
Member

This is also quite a bold step forward to claim that we have the "one true" Duration type, so we should proceed cautiously. This module, however, seems to be of high quality, so I don't expect that to be too much of a problem.

In general the documentation of this module is pretty scarce, and would be nice to beef up things like the documentation on Duration, the module, and cases like why new can fail.

@huonw
Copy link
Member

huonw commented Jul 24, 2014

The name "time" implies functionality like getting the current time

Presumably this module can/will become the facade for what is now libtime (i.e. will be extended so that the time name fits properly).

pub static MAX_DAYS: i32 = i32::MAX;

static NANOS_PER_SEC: i32 = 1_000_000_000;
static SECS_PER_DAY: i32 = 86400;
Copy link
Contributor

Choose a reason for hiding this comment

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

There's precedence for the name NSEC_PER_SEC, so it might make sense to use that here.

Why SECS_PER_DAY? You're not using a static for the other durations, you're using magic numbers instead. NSEC_PER_SEC makes sense because it's easy to lose track of the right number of zeros, but the numbers like 60, 3600, 86400 are much more recognizable and don't necessarily need a static. If you want to keep this static, then you should add ones for 60, 3600. You should also add e.g. MSEC_PER_NSEC and USEC_PER_SEC instead of using magic numbers there as well.

@lilyball
Copy link
Contributor

Like @alexcrichton, I'm not thrilled about the foo_ms functions, I'd rather see them go away (since the names are being reused they can't merely be deprecated).

I'd also like to see a mechanism to convert a Duration back into a numeric value with a given period. I see three general approaches that could work here:

  1. A bunch of methods, to_ms(), to_ns(), etc (which would all return signed values).

  2. Phantom types for each period (that all implement a trait that provides the conversion between the period and nanoseconds, which would incidentally allow users to define custom periods if that's helpful). This is nice because it could get rid of the plethora of existing constructor methods too, leaving just

    pub fn new<T: Period>(period: T, value: i64) -> Duration;
    pub fn from_parts(days: i32, seconds: u32, nanoseconds: u32) -> Duration;

    Extracting the value would then look like:

    pub fn ticks<T: Period>(&self, period: T) -> i64;
  3. Phantom types for each period again, but make Duration be specialized on it instead. This one would be similar to C++'s std::chrono::duration. Not sure if there's a compelling reason to do this over approach 2.

I think approach 2 might be the best.

lifthrasiir added a commit to chronotope/chrono that referenced this pull request Jul 25, 2014
lifthrasiir added a commit to chronotope/chrono that referenced this pull request Jul 25, 2014
…X}_{DAYS,YEAR}`.

the minimum and maximum for `DateZ` and `Duration` is now provided via
dedicated constants `{date,duration}::{MIN,MAX}`, much like built-in
`std::int` and others. they also now implements `std::num::Bounded`.

cf. rust-lang/rust#15934
lifthrasiir added a commit to chronotope/chrono that referenced this pull request Jul 25, 2014
ISO 8601 allows for both but prefers `,`, which was a main reason
for its use in rust-chrono. but this is too alien given other usages of
decimal points in Rust, so I guess it is better to switch to `.`.
@lifthrasiir
Copy link
Contributor

I didn't notice this issue until today, sorry for late replies. Many design issues in rust-chrono are there because I've experimented with different designs (and forgot to revert); feel free to cricitize. As a first step, I've fixed some outstanding issues in rust-chrono.

@aturon
Copy link
Member

aturon commented Aug 12, 2014

OK, looks like all major and many minor comments addressed. Thanks for pushing on this! Let's send it to bors and change the representation in follow-up work.

@aturon
Copy link
Member

aturon commented Aug 13, 2014

@brson About the test failure here -- I just landed a patch that checks for deprecation both cross-crate and crate-locally (whereas other stability lints like the unstable lint are checked only cross-crate). This has been turning up various uses of deprecated items. Looks like that's what happened here.

Taken from rust-chrono[1]. Needed for timers per rust-lang#11189.
Experimental.

[1]: https://github.com/lifthrasiir/rust-chrono
Rename io::timer::sleep, Timer::sleep, Timer::oneshot,
Timer::periodic, to sleep_ms, oneshot_ms, periodic_ms. These functions
all take an integer and interpret it as milliseconds.

Replacement functions will be added that take Duration.

[breaking-change]
From rust-chrono 4f34003e03e259bd5cbda0cb4d35325861307cc6
This is a workaround for having to write `Zero::zero` and will
be solved at the language level someday.
Add tests. Also fix a bunch of broken time tests.
This is only breaking if you were previously specifying a duration
of zero for some mysterious reason.

[breaking-change]
Put `Duration` in `time::duration`, where the two constants can
be called just `MAX` and `MIN`. Reexport from `time`.
This provides more room for the time module to expand.
These all expose the underlying data representation and are
not the most convenient way of instantiation anyway.
bors added a commit that referenced this pull request Aug 13, 2014
Currently, the Timer methods take an integer number of ms. This is considered a bug because a) types, b) some timers have ns precision.

This plucks the `Duration` type from [rust-chrono](https://github.com/lifthrasiir/rust-chrono), plops it into `std::time`,  and replaces the arguments to `sleep`, `oneshot`, and `periodic` timers with it. It leaves the old methods intact as `sleep_ms`, `oneshot_ms`, and `periodic_ms`, for convenience.

Closes #11189.

cc @lifthrasiir @aturon @kballard @alexcrichton
@bors bors closed this Aug 14, 2014
mwhittaker added a commit to mwhittaker/raft that referenced this pull request Aug 15, 2014
[This pull request][1] changed the timer function signatures. I make the
code build again.

[1]: rust-lang/rust#15934
lnicola pushed a commit to lnicola/rust that referenced this pull request May 26, 2024
Clear diagnostics only after new ones were received

Closes rust-lang#15934

This adds a flag inside the global state which controls when old diagnostics are cleared. Now, old diagnostics should be cleared only after at least one new diagnostic is available.
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.

Timer functions should maybe not take milliseconds as an argument
8 participants