Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Replace rounding-error prone floating point code with robust integer code #36

Closed
wants to merge 2 commits into from

Conversation

plugwash
Copy link

When this package was uploaded to Debian we noticed test failures on i386.

Debian i386 uses the x87 floating point unit which is known to have slightly different floating point behavior from other architectures due to excess precision in it's floating point registers. However when I made the testsuite more thorough I found rounding errors on amd64 as well.

 ---- test_duration_ymdhms stdout ----
 thread 'test_duration_ymdhms' panicked at 'assertion failed: `(left == right)`
   left: `Ok(YMDHMS { year: 1, month: 2, day: 3, hour: 4, minute: 5, second: 6, millisecond: 700 })`,
  right: `Ok(YMDHMS { year: 1, month: 2, day: 3, hour: 4, minute: 5, second: 6, millisecond: 699 })`', tests/lib.rs:977:5

---- test_millisecond stdout ----
thread 'test_millisecond' panicked at 'assertion failed: `(left == right)`
  left: `Ok(Time { hour: 16, minute: 43, second: 0, millisecond: 120, tz_offset_hours: 0, tz_offset_minutes: 0 })`,
 right: `Ok(Time { hour: 16, minute: 43, second: 0, millisecond: 119, tz_offset_hours: 0, tz_offset_minutes: 0 })`', tests/lib.rs:36:5

The problem is that sometimes when a decimal number is converted to floating point and multiplied by 1000 the result is slightly smaller than expected due to floating point rounding. When the resulting slightly smaller number is rounded towards zero in the conversion to integer the result is a number one smaller than expected.

My fix was to avoid floating point completely and implement the milliseconds parsing using purely slicing and integer math.

@badboy badboy self-requested a review August 23, 2021 08:41
Copy link
Owner

@badboy badboy left a comment

Choose a reason for hiding this comment

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

Change looks good. Minimal nits before I can merge it.

@plugwash
Copy link
Author

Updated per review comments.

@plugwash plugwash requested a review from badboy August 24, 2021 11:11
@badboy
Copy link
Owner

badboy commented Jul 29, 2022

Merged as 24a8476

@badboy badboy closed this Jul 29, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants