Replace std::to_string with locale-independent alternative

Empact commented at 4:19 am on February 13, 2020: member

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

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

fanquake added the label Refactoring on Feb 13, 2020

Empact force-pushed on Feb 13, 2020

Empact commented at 4:32 am on February 13, 2020: member

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

DrahtBot commented at 8:46 am on February 13, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18147 (qt: Kill the locale dependency bug class by not allowing Qt to mess with LC_NUMERIC by practicalswift)
#18130 (Replace uses of boost::trim* with locale-independent alternatives by Empact)
#18077 (net: Add NAT-PMP port forwarding support by hebasto)
#17681 (wallet: Keep inactive seeds after sethdseed and derive keys from them as needed by achow101)
#16127 (More thread safety annotation coverage by ajtowns)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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 commented at 2:17 pm on February 13, 2020: contributor

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.

in src/rpc/util.cpp:492 in 5be0a882c5 outdated

497@@ -497,7 +498,7 @@ std::string RPCHelpMan::ToString() const
498         if (i == 0) ret += "\nArguments:\n";
499 
500         // Push named argument name and description
501-        sections.m_sections.emplace_back(std::to_string(i + 1) + ". " + arg.m_name, arg.ToDescriptionString());
502+        sections.m_sections.emplace_back(::ToString(i + 1) + ". " + arg.m_name, arg.ToDescriptionString());

practicalswift commented at 2:43 pm on February 13, 2020:

I think this is the only one where ::ToString(…) . Please change the others to simply ToString(…) :)

emilengler commented at 5:35 pm on February 19, 2020:

Maybe I got it wrong but is the RPC interface locale independent anyways?

practicalswift commented at 7:04 pm on February 19, 2020:

The intention is that the RPC interface should be 100% locale independent :)

emilengler commented at 5:01 pm on February 20, 2020:

Ok then never mind.

in src/Makefile.am:227 in 5be0a882c5 outdated

223@@ -224,6 +224,7 @@ BITCOIN_CORE_H = \
224   util/rbf.h \
225   util/settings.h \
226   util/string.h \
227+  util/string_stream.h \

practicalswift commented at 2:44 pm on February 13, 2020:

Put this in util/string.h instead. No need to introduce a new file.

in src/util/string_stream.h:21 in 5be0a882c5 outdated

16+template <typename T>
17+typename std::enable_if<std::is_integral<T>::value, std::string>::type ToString(const T t) {
18+  std::ostringstream oss;
19+  oss.imbue(std::locale::classic());
20+  oss << t;
21+  return oss.str();

practicalswift commented at 2:44 pm on February 13, 2020:

Please run clang-format -i src/util/string_stream.h to get the correct indentation and formatting.

in src/util/string_stream.h:17 in 5be0a882c5 outdated

12+
13+/**
14+ * Locale-independent version of std::to_string
15+ */
16+template <typename T>
17+typename std::enable_if<std::is_integral<T>::value, std::string>::type ToString(const T t) {

practicalswift commented at 2:48 pm on February 13, 2020:

Remove the enable_if: we need to enable this for all types supported (the floating points in particular).

That ToString doesn’t print a lot of trailing zeroes is a feature and not a bug. You can note the difference in the comment though :)

Since std::to_string is locale dependent there should be no assumption regarding formatting regardless, so no harm done by not printing trailing zeroes. Also all current code sites are safe.

in test/lint/lint-locale-dependence.sh:17 in 5be0a882c5 outdated

13@@ -15,9 +14,6 @@ KNOWN_VIOLATIONS=(
14     "src/qt/optionsmodel.cpp.*std::to_string"
15     "src/qt/rpcconsole.cpp:.*atoi"
16     "src/rest.cpp:.*strtol"
17-    "src/rpc/net.cpp.*std::to_string"
18-    "src/rpc/rawtransaction.cpp.*std::to_string"
19-    "src/rpc/util.cpp.*std::to_string"
20     "src/test/addrman_tests.cpp.*std::to_string"

practicalswift commented at 2:49 pm on February 13, 2020:

Please replace all std::to_string with ToString. No need to do this partially. All call sites are safe.

emilengler commented at 5:36 pm on February 19, 2020: contributor

Concept ACK

promag commented at 7:54 am on February 21, 2020: member

Concept ACK.

practicalswift commented at 12:54 pm on March 6, 2020: contributor

@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 (review)).

Empact force-pushed on Mar 9, 2020

Empact commented at 5:49 pm on March 9, 2020: member

Rebased and incorporated your feedback, @practicalswift

practicalswift commented at 6:46 pm on March 9, 2020: contributor

ACK 97eefd7c25f03e898071171eacc45ac5fb5ada6b

DrahtBot added the label Needs rebase on Mar 11, 2020

Empact force-pushed on Mar 11, 2020

DrahtBot removed the label Needs rebase on Mar 11, 2020

Empact commented at 1:24 am on March 13, 2020: member

Rebased

in src/util/string.h:58 in 0add74b0b5 outdated

53@@ -52,4 +54,16 @@ NODISCARD inline bool ValidAsCString(const std::string& str) noexcept
54     return str.size() == strlen(str.c_str());
55 }
56 
57+/**
58+ * Locale-independent version of ToString

sipa commented at 2:06 am on March 13, 2020:

0 * Locale-independent version of std::to_string

in src/util/string.h:61 in 0add74b0b5 outdated

53@@ -52,4 +54,16 @@ NODISCARD inline bool ValidAsCString(const std::string& str) noexcept
54     return str.size() == strlen(str.c_str());
55 }
56 
57+/**
58+ * Locale-independent version of ToString
59+ */
60+template <typename T>
61+std::string ToString(const T t)

sipa commented at 2:08 am on March 13, 2020:

Not really necessary for the simple types in all current call sites, but for future-proofing, const T& t is better to avoid copying expensive types.

ajtowns commented at 8:16 am on March 13, 2020:

And should be inlined anyway, so the extra reference for simple types should get optimised out

Empact commented at 7:24 pm on March 14, 2020:

Yeah did not originally due to the definition of std::to_string, but I agree it makes sense for future-proofing.

sipa commented at 2:11 am on March 13, 2020: member

ACK 0add74b0b5e11124612e6d07b3f68266a4ace66e, minor nits

practicalswift commented at 8:07 am on March 13, 2020: contributor

ACK 0add74b0b5e11124612e6d07b3f68266a4ace66e

Replace std::to_string with locale-independent alternative d056df033a

Empact force-pushed on Mar 14, 2020

Empact commented at 7:24 pm on March 14, 2020: member

Addressed nits.

practicalswift commented at 10:36 pm on March 14, 2020: contributor

ACK d056df033a1e88554f7cc39dd709a87b17cb49df

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

laanwj commented at 7:11 pm on March 25, 2020: member

ACK d056df033a1e88554f7cc39dd709a87b17cb49df

So for tinyformat, do we want to do this as well?

0 // Added for Bitcoin Core
1 template<typename... Args>
2 std::string format(const std::string &fmt, const Args&... args)
3 {
4     std::ostringstream oss;
5+    oss.imbue(std::locale::classic());
6     format(oss, fmt.c_str(), args...);
7     return oss.str();
8 }

laanwj merged this on Mar 25, 2020

laanwj closed this on Mar 25, 2020

practicalswift commented at 10:16 pm on March 25, 2020: contributor

@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 referenced this in commit 2ca9e861e3 on Mar 28, 2020

Fabcien referenced this in commit 47a5b8aad9 on Jan 12, 2021

DrahtBot locked this on Feb 15, 2022

Replace std::to_string with locale-independent alternative #18134

Conflicts