util: Make our stringstream usage locale independent #18432

pull practicalswift wants to merge 3 commits into bitcoin:master from practicalswift:cpp-locale changing 13 files +51 −28

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 12: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
practicalswift force-pushed on Mar 26, 2020
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.

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 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 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 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
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

Contributors

Labels

Linked (view graph)

#18134 Replace std::to_string with locale-independent alternative #18281 Low level functions ScriptToAsmStr (core_io), i64tostr (strencodings) and itostr (strencodings) are locale dependent

util: Make our stringstream usage locale independent #18432

Conflicts