-
Notifications
You must be signed in to change notification settings - Fork 170
Editorial: Check total nanoseconds in IsValidDuration #3186
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
Conversation
Make it clear that it's acceptable for an implementation to use 128-bit integers and do the validity check in terms of nanoseconds.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3186 +/- ##
=======================================
Coverage 96.49% 96.49%
=======================================
Files 22 22
Lines 10305 10305
Branches 1846 1846
=======================================
Hits 9944 9944
Misses 311 311
Partials 50 50 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ptomato
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
ptomato
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @Manishearth
spec/duration.html
Outdated
| 1. NOTE: The above step cannot be implemented directly using floating-point arithmetic. Multiplying by 10<sup>-3</sup>, 10<sup>-6</sup>, and 10<sup>-9</sup> respectively may be imprecise when _milliseconds_, _microseconds_, or _nanoseconds_ is an unsafe integer. This multiplication can be implemented in C++ with an implementation of `std::remquo()` with sufficient bits in the quotient. String manipulation will also give an exact result, since the multiplication is by a power of 10. | ||
| 1. If abs(_normalizedSeconds_) ≥ 2<sup>53</sup>, return *false*. | ||
| 1. Let _normalizedNanoseconds_ be _days_ × 86,400 × 10<sup>9</sup> + _hours_ × 3600 × 10<sup>9</sup> + _minutes_ × 60 × 10<sup>9</sup> + _seconds_ × 10<sup>9</sup> + ℝ(𝔽(_milliseconds_)) × 10<sup>6</sup> + ℝ(𝔽(_microseconds_)) × 10<sup>3</sup> + ℝ(𝔽(_nanoseconds_)). | ||
| 1. NOTE: The above step cannot be implemented directly using 64-bit floating-point arithmetic. Multiplying by 10<sup>-3</sup>, 10<sup>-6</sup>, and 10<sup>-9</sup> respectively may be imprecise when _milliseconds_, _microseconds_, or _nanoseconds_ is an unsafe integer. The step can be implemented using 128-bit integers. It could also be implemented in C++ with an implementation of `std::remquo()` with sufficient bits in the quotient. String manipulation will also give an exact result, since the multiplication is by a power of 10. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"using 128 bit integers and performing all arithmetic on nanosecond values" maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good; changed in 64d0424
10fe6da to
5692cfd
Compare
Make it clear that it's acceptable for an implementation to use 128-bit integers and do the validity check in terms of nanoseconds.
Closes #3177