-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Implement glibc ext for sec, min, and hour #3271
Conversation
b9f8bf1
to
0f10801
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Overall looks good, just minor comments/questions inline.
@@ -664,6 +664,30 @@ enum class numeric_system { | |||
alternative | |||
}; | |||
|
|||
// Glibc extensions for formatting numeric values. | |||
enum class pad_type { | |||
unspecified, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need unspecified? It looks like it's always handled as zero and can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the current codebase, yes.
// Pad a numeric result string with zeros even if the conversion specifier | ||
// character uses space-padding by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which conversion specifiers use space padding by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, the command date "+%10a"
prints Tue
on Ubuntu 22.04.1.
If we were to implement the glibc extension for the optional width, we might need to consider this padding. Otherwise, we don't have space padding in the current codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShawnZhong, thanks for the PR. It looks good overall but I have some minor questions (inline).
This missed some things:
|
Fix #2959