<chrono: Rewrite %r spec for C locale #1865
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
put_time(%r) does the wrong thing when we use the C locale, due to some internal machinery. It could get fixed further down level, but that change is a lot more impactful. Instead, we simply rewrite %r when the C locale is used in chrono. This basically has to do with the way that _Strftime, _Gettnames, and expand_time work together. _Gettnames returns a copy of its data and expand_time figures out the locale based on pointer comparison.
mnatsuhara
approved these changes
Apr 20, 2021
Contributor
mnatsuhara
left a comment
mnatsuhara
left a comment
There was a problem hiding this comment.
My single comment isn't blocking, otherwise looks good!
Comment on lines
+6004
to
+6009
| // put_time uses _Strftime in order to bypass reference-counting that locale uses. This function | ||
| // takes the locale information by pointer, but the pointer (from _Gettnames) returns a copy. | ||
| // _Strftime delegates to other functions but eventually (for the C locale) has the %r specifier | ||
| // rewritten. It checks for the locale by comparing pointers, which do not compare equal as we have | ||
| // a copy of the pointer instead of the original. Therefore, we replace %r for the C locale | ||
| // ourselves. |
Contributor
There was a problem hiding this comment.
If we're reporting this as a UCRT bug (which I think was our conclusion in the internal chat), should this be marked as // TRANSITION, ... with a bug number? Theoretically, we won't have to special-case this forever!
Contributor
Author
There was a problem hiding this comment.
It's not exactly a UCRT bug. They can't really tell we mean the C locale when we pass it in through _Strftime because they are fundamentally different pointers. And we are the ones trying to circumvent the ref counting of locale. Maybe we can fix time_put? People might rely on the %r behavior though...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
put_time(%r) does the wrong thing when we use the C locale, due to some
internal machinery. It could get fixed further down level, but that
change is a lot more impactful. Instead, we simply rewrite %r when the C
locale is used in chrono.
This basically has to do with the way that _Strftime, _Gettnames, and
expand_time work together. _Gettnames returns a copy of its data and
expand_time figures out the locale based on pointer comparison.