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

fmtr-text: do not round up time #45

Merged
merged 1 commit into from
Jul 26, 2024
Merged

Conversation

f00b4r0
Copy link
Contributor

@f00b4r0 f00b4r0 commented Jul 23, 2024

The current code rounds (up) milliseconds when dividing the system-provided microsecond time. Furthermore, if the result of the rounding exceeds 1000ms, it increments the system-provided seconds.

This is wrong as it effectively "pushes time forward", reporting a future time that is not passed at the time of the timestamp collection.

Time should never be rounded, time scale divisions should always be truncated to keep past, present and future in the right order.

This patch fixes this by removing the incorrect time adjustment. tv_usec is guaranteed to always be in the range [0, 999,999]. This also uses integer-based computation instead of (expensive) FP64.

The current code rounds (up) milliseconds when dividing the
system-provided microsecond time. Furthermore, if the result of the
rounding exceeds 1000ms, it increments the system-provided seconds.

This is wrong as it effectively "pushes time forward", reporting a
future time that is not passed at the time of the timestamp collection.

Time should never be rounded, time scale divisions should always be
truncated to keep past, present and future in the right order.

This patch fixes this by removing the incorrect time adjustment.
tv_usec is guaranteed to always be in the range [0, 999,999]. This also
uses integer-based computation instead of (expensive) FP64.
@szpajder szpajder merged commit f83a6d3 into szpajder:unstable Jul 26, 2024
3 of 4 checks passed
@szpajder
Copy link
Owner

Merged, thanks!

@f00b4r0 f00b4r0 deleted the time branch July 27, 2024 09:58
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.

2 participants