Skip to content

util: Make our stringstream usage locale independent #18432

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
Closed
wants to merge 3 commits into from
Closed

util: Make our stringstream usage locale independent #18432

wants to merge 3 commits into from

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Mar 25, 2020 •
edited
Loading

Make our stringstream usage locale independent.

As discussed with laanwj in #18134 (comment). Fixes #18281.

sipa
sipa reviewed Mar 25, 2020
View reviewed changes
@practicalswift practicalswift force-pushed the cpp-locale branch 3 times, most recently from 64adfe1 to b603652 Compare March 26, 2020 00:40
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 26, 2020 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot mentioned this pull request Mar 26, 2020
Closed
maflcko
maflcko reviewed Mar 26, 2020
View reviewed changes
@practicalswift practicalswift force-pushed the cpp-locale branch 2 times, most recently from 2ae47c4 to 0a46909 Compare March 26, 2020 19:07
@practicalswift practicalswift changed the title util: Make current implicit C++ locale assumption in tfm::format(...) explicit util: Make our stringstream usage locale independent. Formalize locale assumptions. Mar 26, 2020
@practicalswift practicalswift changed the title util: Make our stringstream usage locale independent. Formalize locale assumptions. util: Make our stringstream usage locale independent Mar 29, 2020
@practicalswift
Copy link
Contributor Author

@sipa @laanwj Dropped the "util: Make implicit C++ locale assumptions explicit" commit. The current PR looks good? :)

maflcko
maflcko reviewed Mar 29, 2020
View reviewed changes
@practicalswift
Copy link
Contributor Author

@MarcoFalke

Would it be possible to add a static std::stringstream ClassicStringStream() { std::stringstream s; s.imbue(); return s; } to our str util library, so that call sites have a lower LOC-overhead?

Added! Please re-review :)

maflcko
maflcko approved these changes Mar 29, 2020
View reviewed changes
@practicalswift practicalswift force-pushed the cpp-locale branch 2 times, most recently from a512f39 to 57092aa Compare March 29, 2020 15:24
@practicalswift
Copy link
Contributor Author

@MarcoFalke Feedback addressed. Please re-review :)

@practicalswift practicalswift mentioned this pull request Apr 1, 2020
Closed
This was referenced Apr 12, 2020
Closed
Closed
@practicalswift
Copy link
Contributor Author

@MarcoFalke Would you mind reviewing again? Your feedback has been addressed :)

maflcko
maflcko reviewed Apr 15, 2020
View reviewed changes
@DrahtBot DrahtBot mentioned this pull request Apr 16, 2020
Merged
@practicalswift practicalswift force-pushed the cpp-locale branch 2 times, most recently from fd3dcaf to 6744bfd Compare April 18, 2020 21:14
@practicalswift
Copy link
Contributor Author

Rebased :)

@practicalswift practicalswift force-pushed the cpp-locale branch 2 times, most recently from ac170a9 to 5d166eb Compare April 19, 2020 19:32
@DrahtBot DrahtBot mentioned this pull request Apr 20, 2020
Merged
@practicalswift
Copy link
Contributor Author

practicalswift commented Apr 29, 2020 •
edited
Loading

Any interest in avoiding locale dependence? If not I'll close :)

@DrahtBot DrahtBot mentioned this pull request May 9, 2020
Merged
@practicalswift practicalswift deleted the cpp-locale branch April 10, 2021 19:40
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

@sipa sipa sipa left review comments

+1 more reviewer

@maflcko maflcko maflcko approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Low level functions ScriptToAsmStr (core_io), i64tostr (strencodings) and itostr (strencodings) are locale dependent
6 participants
@practicalswift @DrahtBot @laanwj @sipa @maflcko @fanquake