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: use C locale by default #1297

Merged
merged 1 commit into from
Jul 17, 2024
Merged

fix: use C locale by default #1297

merged 1 commit into from
Jul 17, 2024

Conversation

SGSSGene
Copy link
Contributor

fix #1282

This explicitly sets locale 'C' to all std::stringstream objects that output YAML.

@jbeder
Copy link
Owner

jbeder commented Jul 17, 2024

In terms of verification: the issue #1282 talks about "running under" LC_ALL=en_US.UTF-8 and similar. How does the test you wrote here relate to that? Is that an environment variable? (Can that be set/unset in the test?)

@SGSSGene
Copy link
Contributor Author

SGSSGene commented Jul 17, 2024

In terms of verification: the issue #1282 talks about "running under" LC_ALL=en_US.UTF-8 and similar. How does the test you wrote here relate to that? Is that an environment variable? (Can that be set/unset in the test?)

So, by default the locale() is set to "" at startup, meaning LC_ALL has no influence. But in there case someone is setting std::locale::global(std::locale("")) which changes the global setting to follow LC_ALL/LC_NUMERIC/etc. Instead of setting it to std::locale("") which reads from LC_ALL we can set it to a value of our wishes.

A locale set to en_US.UTF-8 would print 2014 as 2,014
Setting the same to de_DE.UTF-8 would print 2014" as 2.014`. Both things we really do not want.

So instead I overwrite locally for every std::stringstream it explicitly to "C", which I believe is the behavior we want.

Edit:
This test will fail (by throwing an exception) if en_US.UTF-8 is not available.

@jbeder jbeder merged commit b95aa14 into jbeder:master Jul 17, 2024
33 checks passed
@SGSSGene SGSSGene deleted the fix/locales branch July 17, 2024 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Numbers are emitted wrongly under non-C locales.
2 participants