Skip to content

Row.c code shrink and adding a new format for Row_printNanoseconds() #1579

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Row.c code shrink and adding a new format for Row_printNanoseconds() #1579

wants to merge 4 commits into from
+19 −14

Conversation

Explorer09
Copy link
Contributor

The first three commits are code shrinks.
The last one commit adds a new format for Row_printNanoseconds() because I feel I missed a chance of allowing one extra digit of precision to be printed.
The last commit is optional. If people feel the new format is not worth it, I can drop that commit.

(Follow-up of #1425)

BenBE
BenBE approved these changes Jan 2, 2025
View reviewed changes
Copy link
Member

@BenBE BenBE left a comment

Choose a reason for hiding this comment

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

LGTM. Although a bit unconventional to skip from ns to ms right away … Also, even though we have Unicode, for units I'd still prefer to use us unconditionally.

@Explorer09
Copy link
Contributor Author

LGTM. Although a bit unconventional to skip from ns to ms right away … Also, even though we have Unicode, for units I'd still prefer to use us unconditionally.

"1.0000ms" vs. "1000.0us" ...

These are same precision for showing the fractions of a second. I don't know which one looks better.

Note that the time units in the GPUMeter would have to use us anyway (due to shorter width of the field). I have thought of adding the code to display μs there.

@BenBE
Copy link
Member

BenBE commented Jan 3, 2025

I'm neutral either way …

@Explorer09 Explorer09 force-pushed the print-time-format branch 2 times, most recently from 4ff9f7a to 54d77d4 Compare January 29, 2025 08:48
@fasterit
Copy link
Member

Can we have one "format times" code as in #1583 and this combined?

@fasterit
Copy link
Member

https://en.wikipedia.org/wiki/Microsecond ... "Its symbol is μs, sometimes simplified to us when Unicode is not available."
No need to ignore the unit. It's 2025 and we have #ifdef HAVE_LIBNCURSESW 🎉 .

@Explorer09
Copy link
Contributor Author

Can we have one "format times" code as in #1583 and this combined?

The output of #1583 is an ASCII string (char []), potentially allowing UTF-8.
The output of this one (#1579 and #1425) is a RichString buffer.
So, nope. While I wish the two routines be merged, it's impossible due to the difference of output object types.

https://en.wikipedia.org/wiki/Microsecond ... "Its symbol is μs, sometimes simplified to us when Unicode is not available." No need to ignore the unit. It's 2025 and we have #ifdef HAVE_LIBNCURSESW 🎉 .

Ask @BenBE for this. Actually I wished the Greek letter mu (μ, U+03BC) be used when it's available, but a previous comment of BenBE said something about using us unconditionally...

@BenBE
Copy link
Member

BenBE commented Feb 17, 2025

Using us instead of µs was suggested in my comment mostly for two reasons:

  1. Avoids distinguishing cases in the code whether Unicode is available or not.
  2. Avoids issues with printing on the terminal when locales are badly configured.

At least the second situation can occur easily when working with screen and tmux over SSH when some of the environment variables are forwarded incorrectly or mixed up due to diverging settings on different computers. Using us, while not being canonical, avoids both these issues and is usually understood just as well.

@fasterit: For me it's not a pressing issue and I'm totally indifferent about which one we use in the end.

@Explorer09
Copy link
Contributor Author

Using us instead of µs was suggested in my comment mostly for two reasons:

  1. Avoids distinguishing cases in the code whether Unicode is available or not.
  2. Avoids issues with printing on the terminal when locales are badly configured.

At least the second situation can occur easily when working with screen and tmux over SSH when some of the environment variables are forwarded incorrectly or mixed up due to diverging settings on different computers. Using us, while not being canonical, avoids both these issues and is usually understood just as well.

I'm personally neutral on this, because after #1598 it would be easier to add the μ symbol support while having u as an appropriate fallback. It's just extra code that some people might consider it not worth it.

@Explorer09 Explorer09 force-pushed the print-time-format branch 4 times, most recently from e205322 to de0f493 Compare February 21, 2025 19:37
@Explorer09 Explorer09 force-pushed the print-time-format branch 2 times, most recently from 61da73e to d0863d7 Compare March 8, 2025 08:55
@Explorer09 Explorer09 force-pushed the print-time-format branch 2 times, most recently from 871e921 to 206a2e6 Compare April 2, 2025 23:00
@Explorer09 Explorer09 force-pushed the print-time-format branch 4 times, most recently from b3eca7c to e7fcca1 Compare May 15, 2025 21:17
@Explorer09 Explorer09 force-pushed the print-time-format branch 2 times, most recently from 6079c6e to 32fc21c Compare May 21, 2025 21:28
@Explorer09 Explorer09 force-pushed the print-time-format branch 2 times, most recently from 4c8039d to 4958b20 Compare June 3, 2025 22:14
@Explorer09
Row_printKBytes() code shrink
0babe23 
* Reduce length of the format strings by removing redundant field
  width specifications (e.g. "%1u" -> "%u").
* Merge some identical function calls in conditionals.

Signed-off-by: Kang-Che Sung <[email protected]>
Explorer09 added 3 commits June 4, 2025 18:11
@Explorer09
Row_printTime() code shrink
011fa4a 
Shorten a format string ("%1ud" -> "%ud").

Signed-off-by: Kang-Che Sung <[email protected]>
@Explorer09
Row_printNanoseconds() code shrink
2e4ab56 
Merge two xSnprintf() calls within the function. When the time value to
print is less than one second, the "%.u" format allows us to print no
digits in the seconds field.

Signed-off-by: Kang-Che Sung <[email protected]>
@Explorer09
Add a new printing format to Row_printNanoseconds()
b49c47a 
Add a new format: "1.0000ms" - "9.9999ms".
Previously they would print as ".001000s" and ".009999s", and this new
format adds one extra digit of precision.
Note that I print the unit as "ms" rather than microseconds; this saves
code for deciding whether to print the Greek "mu" or the Latin "u".

Row_printNanoseconds() now prints this list of formats:
"     0ns", "999999ns", "1.0000ms", "9.9999ms", ".010000s", ".999999s",
"1.00000s", "59.9999s", "1:00.000", "9:59.999", "10:00.00" ...

Signed-off-by: Kang-Che Sung <[email protected]>
@Explorer09 Explorer09 force-pushed the print-time-format branch from 4958b20 to b49c47a Compare June 4, 2025 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenBE BenBE BenBE approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Explorer09 @BenBE @fasterit