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
Explorer09 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

Explorer09 wants to merge 4 commits into from
+19 −14

Conversation

@Explorer09
Copy link
Contributor

@Explorer09 Explorer09 commented Jan 1, 2025

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

Explorer09 commented Jan 2, 2025

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

fasterit commented Jan 29, 2025

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

@fasterit
Copy link
Member

fasterit commented Jan 29, 2025

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

Explorer09 commented Jan 29, 2025

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

Explorer09 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.

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 3 times, most recently from 4958b20 to b49c47a Compare June 4, 2025 10:11
@Explorer09 Explorer09 force-pushed the print-time-format branch 3 times, most recently from d68a60e to 6447a95 Compare October 17, 2025 17:19
@Explorer09 Explorer09 force-pushed the print-time-format branch 3 times, most recently from b0fd573 to 33ceb06 Compare October 28, 2025 12:33
@Explorer09 Explorer09 force-pushed the print-time-format branch 3 times, most recently from 1c3b06d to 5707d2e Compare November 15, 2025 20:07
@Explorer09 Explorer09 force-pushed the print-time-format branch 4 times, most recently from dd291a0 to 78065ef Compare December 2, 2025 17:22
@Explorer09
Row_printKBytes() code shrink
956a631 
* 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
Row_printTime() code shrink
39dbaa8 
Shorten a format string ("%1ud" -> "%ud").

Signed-off-by: Kang-Che Sung <[email protected]>
@Explorer09
Row_printNanoseconds() code shrink
f14abd1 
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()
cc887b8 
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]>
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