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

Fix handling of locale in chrono formatters #1892

Closed
wants to merge 6 commits into from
Closed

Fix handling of locale in chrono formatters #1892

wants to merge 6 commits into from

Conversation

vitaut
Copy link
Contributor

@vitaut vitaut commented May 1, 2021

Make chrono formatting locale-independent by default and add the L specifier for consistency with all other standard formatters. Resolves https://cplusplus.github.io/LWG/issue3547 and #1891.

For example:

C:\test>type test.cc
#include <chrono>
#include <format>
#include <iostream>

int main() {
  std::locale::global(std::locale("ru_RU"));
  std::cout << std::format("{}\n", 0.042);
  std::cout << std::format("{:%S}\n", std::chrono::milliseconds(42));
  std::cout << std::format("{:L%S}\n", std::chrono::milliseconds(42));
}
C:\test>test
0.042
00.042
00,042

@vitaut vitaut requested a review from a team as a code owner May 1, 2021 17:52
stl/inc/chrono Show resolved Hide resolved
@miscco
Copy link
Contributor

miscco commented May 2, 2021

How about you put a test on it

@StephanTLavavej StephanTLavavej added bug Something isn't working chrono C++20 chrono format C++20/23 format labels May 2, 2021
@StephanTLavavej StephanTLavavej linked an issue May 2, 2021 that may be closed by this pull request
@barcharcraz
Copy link
Member

Needs tests, and also probably needs ABI mitigations (that would involve changing mangled names).

Also I don't want to merge this before the LWG issue is approved, it's somewhat of a design change and I'm not sure LWG will adopt the issue.

Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Thanks! Aside from ABI renaming and stylistic nitpicks, this looks good to me.

stl/inc/chrono Show resolved Hide resolved
stl/inc/chrono Outdated Show resolved Hide resolved
@vitaut
Copy link
Contributor Author

vitaut commented May 5, 2021

Switched ostream insertion operators back to using locale per LEWG request and implemented two drive-by fixes:

  1. Use the correct locale when formatting utc_time and similar types (previously the global locale was used).
  2. Use the correct locale when chrono specs are omitted (also the global locale was used).

Also added tests for insertion operators. Note that item 1 is currently untestable (there is no observable change) because %T is broken - it ignores the fractional part of the seconds component. Fixing %T is out of scope of this PR.

@barcharcraz
Copy link
Member

We can't merge this until the paper has been forwarded out of LWG, however we're happy to keep the PR open and continue reviewing changes.

@barcharcraz barcharcraz added blocked on LWG Waiting for WG21 to tell us what to do cxx23 C++23 feature and removed bug Something isn't working labels May 5, 2021
@StephanTLavavej
Copy link
Member

Here's hoping that this paper gets through LWG swiftly 😸

@StephanTLavavej StephanTLavavej removed the blocked on LWG Waiting for WG21 to tell us what to do label Oct 5, 2021
@StephanTLavavej
Copy link
Member

The paper has been voted into the WP: #2237

@vitaut
Copy link
Contributor Author

vitaut commented Oct 5, 2021

I don't plan to work on this PR (although it should be mostly ready), hopefully someone will take over.

@vitaut vitaut closed this Oct 5, 2021
@barcharcraz
Copy link
Member

I'll get this past the finish line now that it's been voted in.

Are you OK with just reopening this PR (and me pushing into it) or do you want to avoid github notifications and have me pick out the commits into a new PR.

@vitaut
Copy link
Contributor Author

vitaut commented Oct 7, 2021

Unfortunately I already removed the repo this PR originated from so I can't reopen. Please create a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chrono C++20 chrono cxx23 C++23 feature format C++20/23 format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

D2372R1: Fixing locale handling in chrono formatters
5 participants