Skip to content

Conversation

Empact
Copy link
Contributor

@Empact Empact commented Feb 13, 2020

Addresses #17866 following practicalswift's suggestion:
#17866 (comment)

Used ::ToString to avoid aliasing issues. Left uses in QT and test.

@Empact Empact force-pushed the 2020-02-to-string branch 2 times, most recently from a2f813d to 5be0a88 Compare February 13, 2020 04:32
@Empact
Copy link
Contributor Author

Empact commented Feb 13, 2020

Limited to integral types to avoid inconsistencies noted in the examples here: https://en.cppreference.com/w/cpp/string/basic_string/to_string

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 13, 2020

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.

@practicalswift
Copy link
Contributor

practicalswift commented Feb 13, 2020

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.

Copy link
Contributor

@cvengler cvengler left a comment

Choose a reason for hiding this comment

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

Concept ACK

@promag
Copy link
Contributor

promag commented Feb 21, 2020

Concept ACK.

@practicalswift
Copy link
Contributor

practicalswift commented Mar 6, 2020

@Empact Are you still working on this PR? :)

I think it would be really good to have ToString(…) in our code base since std::to_string(…) usage keeps popping up in new code. Latest case in #18261 (see #18261 (comment)).

@Empact Empact force-pushed the 2020-02-to-string branch 2 times, most recently from ed2c350 to 97eefd7 Compare March 9, 2020 17:49
@Empact
Copy link
Contributor Author

Empact commented Mar 9, 2020

Rebased and incorporated your feedback, @practicalswift

@practicalswift
Copy link
Contributor

ACK 97eefd7

@Empact
Copy link
Contributor Author

Empact commented Mar 13, 2020

Rebased

Copy link
Member

@sipa sipa left a 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

@practicalswift
Copy link
Contributor

ACK 0add74b

@Empact Empact force-pushed the 2020-02-to-string branch from 0add74b to d056df0 Compare March 14, 2020 19:23
@Empact
Copy link
Contributor Author

Empact commented Mar 14, 2020

Addressed nits.

@practicalswift
Copy link
Contributor

ACK d056df0

Very pleased to see the removal of no less than 16(!) instances of calls to locale dependent functions. Thanks! :)

@laanwj
Copy link
Member

laanwj commented Mar 25, 2020

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 laanwj merged commit 2e97d80 into bitcoin:master Mar 25, 2020
@practicalswift
Copy link
Contributor

practicalswift commented Mar 25, 2020

@laanwj Technically we don't technically that in the tfm::format case since we're already implicitly assuming that the C++ locale is always the classic locale. If that was not the case a lot of things would break as demonstrated in #18281. However I think we should make that implicit assumption explicit - PR coming up! :)

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 28, 2020
…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
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 12, 2021
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants