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

time builtin: align output columns on rare cases #6726

Merged
merged 1 commit into from Mar 14, 2020
Merged

time builtin: align output columns on rare cases #6726

merged 1 commit into from Mar 14, 2020

Conversation

afq984
Copy link
Contributor

@afq984 afq984 commented Mar 9, 2020

  1. When the wall time and cpu time rows has different units
    e.x. running multiple cores
  2. When duration is around 1E3 or 1E6 micros
    printf("%6.2F", 999.995) gives 1000.00 which is 7 digits

The included test looks like this without the modification:

________________________________________________________
Executed in  500.00 micros    fish           external 
   usr time    1.00 secs  1000.00 millis  1000.00 micros 
   sys time    1.00 secs  999.99 millis  500.00 micros

1. When the wall time and cpu time rows has different units
   e.x. running multiple cores
2. When duration is around 1E3 or 1E6 microseconds
   printf("%6.2F", 999.995) gives 1000.00 which is 7 digits
@krobelus
Copy link
Member

When duration is around 1E3 or 1E6 micros
printf("%6.2F", 999.995) gives 1000.00 which is 7 digits

So printf rounds float arguments, it looks like we first need to do that rounding before we know how many digits there are, so we can choose the correct unit.

I imagine printing a float value with sprintf and reading it back with sscanf should do the trick, though there may be more modern solutions.

@afq984
Copy link
Contributor Author

afq984 commented Mar 11, 2020

I imagine printing a float value with sprintf and reading it back with sscanf should do the trick, though there may be more modern solutions.

snprintf with size 0 string would return the number of characters needed for the print.
That is snprintf(0, 0, "%6.2F", 999.995) => 7; snprintf(0, 0, "%6.2F", 999.994) => 6.


I implement it as a check for (>= 1000 and >= 999995) because get_unit takes int64_t, and in master, the only values that trigger the %6.2F overflow are

  • 1000, 1000000 (because of the use of > instead of >=)
  • 999995 to 999999 (because of rounding)

There isn't a 1000.00 secs problem because we switch to 15.00 mins after 900.00 secs.

@krobelus krobelus merged commit f864bd8 into fish-shell:master Mar 14, 2020
@krobelus
Copy link
Member

Nice I didn't know that about snprintf.

I can see now why this is sufficient, merged with thanks!

@krobelus krobelus added the bug Something that's not working as intended label Mar 14, 2020
@krobelus krobelus added this to the fish 3.2.0 milestone Mar 14, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something that's not working as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants