util: Make our stringstream usage locale independent

practicalswift commented at 11:15 pm on March 25, 2020: contributor

Make our stringstream usage locale independent.

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

fanquake added the label Utils/log/libs on Mar 25, 2020

in src/tinyformat.h:1162 in 266e3162f9 outdated

1153@@ -1152,6 +1154,12 @@ TINYFORMAT_FOREACH_ARGNUM(TINYFORMAT_MAKE_FORMAT_FUNCS)
1154 template<typename... Args>
1155 std::string format(const std::string &fmt, const Args&... args)
1156 {
1157+    // Numerous tfm::format(...) callers are implicitly assuming that the output
1158+    // returned is locale independent. Notable examples include i64tostr, itostr
1159+    // and ScriptToAsmStr. Make sure to do a thorough review of all call sites
1160+    // (and other direct/indirect stringstream consumers) if this assumption is
1161+    // ever removed in the future.
1162+    assert(std::locale{} == std::locale::classic());

sipa commented at 11:52 pm on March 25, 2020:

I’d rather just make this explicitly use the classic locale than forcing a global assumption on the code (i.e. oss.imbue(std::locale::classic());).

practicalswift commented at 0:20 am on March 26, 2020:

Fixed!

laanwj commented at 2:56 pm on March 26, 2020:

Agree with @sipa here.

practicalswift force-pushed on Mar 26, 2020

DrahtBot commented at 7:37 am on March 26, 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:

#18918 (wallet: Move salvagewallet into wallettool by achow101)
#18608 (refactor: Remove CAddressBookData::destdata 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.

in src/tinyformat.h:1164 in b6036527be outdated

1153@@ -1153,6 +1154,7 @@ template<typename... Args>
1154 std::string format(const std::string &fmt, const Args&... args)
1155 {
1156     std::ostringstream oss;
1157+    oss.imbue(std::locale::classic());
1158     format(oss, fmt.c_str(), args...);
1159     return oss.str();

MarcoFalke commented at 1:03 pm on March 26, 2020:

How does this have any effect? This is pretty much dead code (most of our format strings are c strings, so the other function is called). Might as well remove the code here to clarify this is not the primary implementation.

0    return format(fmt.c_str(), args...);

laanwj commented at 2:59 pm on March 26, 2020:

Ouch, good catch. I also thought this was the main entry point.

practicalswift commented at 3:24 pm on March 26, 2020:

I’ve now given all functions the same treatment. Please re-review :)

MarcoFalke commented at 2:57 pm on March 29, 2020:

I think this can still be replaced by a single line return format(fmt.c_str(), args...);.

practicalswift commented at 3:23 pm on March 29, 2020:

Fixed!

practicalswift force-pushed on Mar 26, 2020

practicalswift renamed this:
~~util: Make current implicit C++ locale assumption in tfm::format(...) explicit~~
util: Make our stringstream usage locale independent. Formalize locale assumptions.
on Mar 26, 2020

practicalswift force-pushed on Mar 29, 2020

practicalswift renamed this:
~~util: Make our stringstream usage locale independent. Formalize locale assumptions.~~
util: Make our stringstream usage locale independent
on Mar 29, 2020

practicalswift commented at 12:25 pm on March 29, 2020: contributor

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

in src/blockfilter.cpp:247 in 9899c09396 outdated

243@@ -243,6 +244,7 @@ const std::string& ListBlockFilterTypes()
244     static std::once_flag flag;
245     std::call_once(flag, []() {
246             std::stringstream ret;
247+            ret.imbue(std::locale::classic());

MarcoFalke commented at 12:47 pm on March 29, 2020:

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?

practicalswift commented at 2:45 pm on March 29, 2020: contributor

@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 :)

practicalswift force-pushed on Mar 29, 2020

in src/wallet/db.cpp:6 in af8f50b2f7 outdated

2@@ -3,12 +3,12 @@
3 // Distributed under the MIT software license, see the accompanying
4 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
5 
6-#include <wallet/db.h>

MarcoFalke commented at 2:59 pm on March 29, 2020:

The module header is always included first, I think

practicalswift commented at 3:24 pm on March 29, 2020:

Fixed!

MarcoFalke approved

practicalswift force-pushed on Mar 29, 2020

practicalswift commented at 3:25 pm on March 29, 2020: contributor

@MarcoFalke Feedback addressed. Please re-review :)

practicalswift commented at 2:25 pm on April 15, 2020: contributor

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

in src/tinyformat.h:1134 in 57092aad16 outdated

1130@@ -1126,6 +1131,7 @@ template<TINYFORMAT_ARGTYPES(n)>                                          \
1131 std::string format(const char* fmt, TINYFORMAT_VARARGS(n))                \
1132 {                                                                         \
1133     std::ostringstream oss;                                               \
1134+    oss.imbue(std::locale::classic()); /* Added for Bitcoin Core */       \

MarcoFalke commented at 2:33 pm on April 15, 2020:

Have you tried submitting this upstream? Other users might be interested in this, but it might have to be hidden behind a compile time flag.

practicalswift commented at 2:51 pm on April 15, 2020:

My plan is to do that if/when we get this in to Core :)

DrahtBot added the label Needs rebase on Apr 17, 2020

practicalswift force-pushed on Apr 18, 2020

practicalswift commented at 9:15 pm on April 18, 2020: contributor

Rebased :)

DrahtBot removed the label Needs rebase on Apr 18, 2020

practicalswift force-pushed on Apr 19, 2020

DrahtBot added the label Needs rebase on Apr 20, 2020

Use std::locale::classic() when using stringstreams 25cb97000e

util: Add LocaleIndependentStream<T>() and LocaleIndependentStringStream() 2aa0f6fa9a

util: Use LocaleIndependentStringStream() to get a std::stringstream with locale std::locale::classic() 55f5c2c79f

practicalswift force-pushed on Apr 24, 2020

DrahtBot removed the label Needs rebase on Apr 24, 2020

practicalswift commented at 2:44 pm on April 29, 2020: contributor

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

practicalswift closed this on May 12, 2020

practicalswift deleted the branch on Apr 10, 2021

DrahtBot locked this on Aug 16, 2022

util: Make our stringstream usage locale independent #18432

Conflicts