Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upDuration div mul extras #52813
Duration div mul extras #52813
Conversation
This comment has been minimized.
This comment has been minimized.
(rust_highfive has picked a reviewer for you, use r? to override) |
This comment was marked as resolved.
This comment was marked as resolved.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment was marked as resolved.
This comment was marked as resolved.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment was marked as resolved.
This comment was marked as resolved.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment was marked as resolved.
This comment was marked as resolved.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment was marked as resolved.
This comment was marked as resolved.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment was marked as resolved.
This comment was marked as resolved.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
Could you squash all fixups into a single commit? |
9b0d0b5
to
c24fb12
This comment has been minimized.
This comment has been minimized.
Done. |
if !nanos_f64.is_finite() { | ||
panic!("got non-finite value when multiplying duration by float"); | ||
} | ||
if nanos_f64 > (u128::MAX as f64) { |
This comment has been minimized.
This comment has been minimized.
kennytm
Jul 29, 2018
Member
The upper limit is u64::MAX * NANOS_PER_SEC
(1.8 × 1028) which is a much smaller number than u128::MAX
(3.4 × 1038). If you check for the former you don't need the second secs > (u64::MAX as u128)
below.
This comment has been minimized.
This comment has been minimized.
newpavlov
Jul 29, 2018
Author
Contributor
I was conservative about potential edge effects. So you think it will be enough to just check the following condition?
nanos_f64 > ((u64::MAX as u128)*(NANOS_PER_SEC as u128)) as f64
This comment has been minimized.
This comment has been minimized.
|
||
fn mul(self, rhs: f64) -> Duration { | ||
const NPS: f64 = NANOS_PER_SEC as f64; | ||
let nanos_f64 = rhs * (NPS * (self.secs as f64) + (self.nanos as f64)); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
c2eb698
to
9246687
This comment has been minimized.
This comment has been minimized.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
9246687
to
9086139
9086139
to
12d8f27
fn mul(self, rhs: Duration) -> Duration { | ||
const NPS: f64 = NANOS_PER_SEC as f64; | ||
if self.is_sign_negative() { | ||
panic!("duration can not be multiplied by negative float"); |
This comment has been minimized.
This comment has been minimized.
scottmcm
Jul 30, 2018
Member
This will trigger for a NaN with the sign bit set, which feels like it'd be better reported under non-finite, so maybe just use self < 0
here? Also, multiplying a duration by negative zero seems like it should successfully give a zero duration, not a panic.
type Output = Duration; | ||
|
||
fn mul(self, rhs: Duration) -> Duration { | ||
rhs.checked_mul(self).expect("overflow when multiplying scalar by duration") |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
newpavlov
Jul 30, 2018
Author
Contributor
They have different panic messages "multiplying scalar by duration" vs "multiplying duration by scalar"
This comment has been minimized.
This comment has been minimized.
newpavlov
Jul 30, 2018
Author
Contributor
If having different panic messages is not important, then I'll change it and f64
impl to rhs * self
.
5cc5272
to
2ddc2ea
2ddc2ea
to
2c300fa
This comment has been minimized.
This comment has been minimized.
Considering Rust 1.28 release should I change stabilization version to |
This comment has been minimized.
This comment has been minimized.
@newpavlov Yes. |
@@ -30,6 +30,7 @@ const NANOS_PER_MILLI: u32 = 1_000_000; | |||
const NANOS_PER_MICRO: u32 = 1_000; | |||
const MILLIS_PER_SEC: u64 = 1_000; | |||
const MICROS_PER_SEC: u64 = 1_000_000; | |||
const MAX_NANOS_F64: f64 = ((u64::MAX as u128 + 1)*(NANOS_PER_SEC as u128) - 1) as f64; |
This comment has been minimized.
This comment has been minimized.
ollie27
Sep 13, 2018
Contributor
The - 1
here doesn't change the value because f64
doesn't have enough precision so this constant ends up being exactly 1ns greater than the largest possible Duration
. Either this constant needs to be changed to the next smallest f64
or the check below needs to be changed to nanos >= MAX_NANOS_F64
. from_float_secs(2f64.powi(64))
should trigger an overflow but currently it returns a Duration
of 0ns.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@bors: r+ Looks good to me, thanks! |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
@bors: r- er actually, mind filing a tracking issue and filling that in? |
This comment has been minimized.
This comment has been minimized.
Done. |
This comment has been minimized.
This comment has been minimized.
@bors: r+ |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
Duration div mul extras Successor of #52556. This PR adds the following `impl`s: - `impl Mul<Duration> for u32` (to allow `10*SECOND` in addition to `SECOND*10`) - `impl Mul<f64> for Duration` (to allow `2.5*SECOND` vs `2*SECOND + 500*MILLISECOND`) - `impl Mul<Duration> for f64` - `impl MulAssign<f64> for Duration` - `impl Div<f64> for Duration` - `impl DivAssign<f64> for Duration` - `impl Div<Duration> for Duration` (`Output = f64`, can be useful e.g. for `duration/MINUTE`) `f64` is chosen over `f32` to minimize rounding errors. (52 bits fraction precision vs `Duration`'s ~94 bit)
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
@bors retry 3 hour timeout |
…r=alexcrichton Duration div mul extras Successor of rust-lang#52556. This PR adds the following `impl`s: - `impl Mul<Duration> for u32` (to allow `10*SECOND` in addition to `SECOND*10`) - `impl Mul<f64> for Duration` (to allow `2.5*SECOND` vs `2*SECOND + 500*MILLISECOND`) - `impl Mul<Duration> for f64` - `impl MulAssign<f64> for Duration` - `impl Div<f64> for Duration` - `impl DivAssign<f64> for Duration` - `impl Div<Duration> for Duration` (`Output = f64`, can be useful e.g. for `duration/MINUTE`) `f64` is chosen over `f32` to minimize rounding errors. (52 bits fraction precision vs `Duration`'s ~94 bit)
Rollup of 16 pull requests Successful merges: - #52813 (Duration div mul extras) - #53470 (Warn about metadata loader errors) - #54233 (Remove LLVM 3.9 workaround.) - #54257 (Switch wasm math symbols to their original names) - #54258 (Enable fatal warnings for the wasm32 linker) - #54266 (Update LLVM to fix "bool" arguments on PPC32) - #54290 (Switch linker for aarch64-pc-windows-msvc from LLD to MSVC) - #54292 (Suggest array indexing when tuple indexing on an array) - #54295 (A few cleanups and minor improvements to rustc/traits) - #54298 (miri: correctly compute expected alignment for field) - #54317 (Implement the dbg!(..) macro) - #54333 (Update The Book to latest) - #54337 (Remove unneeded clone() from tests in librustdoc) - #54346 (rustc: future-proof error reporting for polymorphic constants in types.) - #54362 (Pass --batch to gdb) - #54367 (Add regression test for thread local static mut borrows) Failed merges: r? @ghost
Rollup of 15 pull requests Successful merges: - #52813 (Duration div mul extras) - #53470 (Warn about metadata loader errors) - #54233 (Remove LLVM 3.9 workaround.) - #54257 (Switch wasm math symbols to their original names) - #54258 (Enable fatal warnings for the wasm32 linker) - #54266 (Update LLVM to fix "bool" arguments on PPC32) - #54290 (Switch linker for aarch64-pc-windows-msvc from LLD to MSVC) - #54292 (Suggest array indexing when tuple indexing on an array) - #54295 (A few cleanups and minor improvements to rustc/traits) - #54298 (miri: correctly compute expected alignment for field) - #54333 (Update The Book to latest) - #54337 (Remove unneeded clone() from tests in librustdoc) - #54346 (rustc: future-proof error reporting for polymorphic constants in types.) - #54362 (Pass --batch to gdb) - #54367 (Add regression test for thread local static mut borrows)
newpavlov commentedJul 28, 2018
•
edited
Successor of #52556.
This PR adds the following
impl
s:impl Mul<Duration> for u32
(to allow10*SECOND
in addition toSECOND*10
)impl Mul<f64> for Duration
(to allow2.5*SECOND
vs2*SECOND + 500*MILLISECOND
)impl Mul<Duration> for f64
impl MulAssign<f64> for Duration
impl Div<f64> for Duration
impl DivAssign<f64> for Duration
impl Div<Duration> for Duration
(Output = f64
, can be useful e.g. forduration/MINUTE
)f64
is chosen overf32
to minimize rounding errors. (52 bits fraction precision vsDuration
's ~94 bit)