-
Notifications
You must be signed in to change notification settings - Fork 37.8k
Replace std::to_string with locale-independent alternative #18134
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
Conversation
a2f813d
to
5be0a88
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
Concept ACK: simple and robust (locale independent) is better than gotcha-filled and fragile (locale dependent). Thanks for working on reducing the usage of locale dependent functions. Hope to see more PR's of this type from you :) Will review. |
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.
Concept ACK
Concept ACK. |
@Empact Are you still working on this PR? :) I think it would be really good to have |
ed2c350
to
97eefd7
Compare
Rebased and incorporated your feedback, @practicalswift |
ACK 97eefd7 |
97eefd7
to
0add74b
Compare
Rebased |
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.
ACK 0add74b, minor nits
ACK 0add74b |
0add74b
to
d056df0
Compare
Addressed nits. |
ACK d056df0 Very pleased to see the removal of no less than 16(!) instances of calls to locale dependent functions. Thanks! :) |
ACK d056df0 So for tinyformat, do we want to do this as well? // Added for Bitcoin Core
template<typename... Args>
std::string format(const std::string &fmt, const Args&... args)
{
std::ostringstream oss;
+ oss.imbue(std::locale::classic());
format(oss, fmt.c_str(), args...);
return oss.str();
} |
@laanwj Technically we don't technically that in the |
…lternative d056df0 Replace std::to_string with locale-independent alternative (Ben Woosley) Pull request description: Addresses bitcoin#17866 following practicalswift's suggestion: bitcoin#17866 (comment) ~Used ::ToString to avoid aliasing issues. Left uses in QT and test.~ ACKs for top commit: practicalswift: ACK d056df0 laanwj: ACK d056df0 Tree-SHA512: 9e6966a9cdd14f4a1a40d9f0fa7c402aed22b2f1ad8681708e22b050d51a91c5d62220a9ec4c425be2d57acf5c964fca87a5e981b5cbff048bc3b6720dae92b7
Summary: > `std::to_string` relies on the current locale for formatting purposes This is a backport of Core [[bitcoin/bitcoin#18134 | PR18134]] and [[bitcoin/bitcoin#17851 | PR17851]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien, majcosta Reviewed By: #bitcoin_abc, Fabien, majcosta Subscribers: majcosta, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8878
Addresses #17866 following practicalswift's suggestion:
#17866 (comment)
Used ::ToString to avoid aliasing issues. Left uses in QT and test.