check std::to_string usage #17866

#17851 added std::to_string to the list of locale dependent functions, along with a large number of usage exceptions. It was pointed out that some of these usages are actually likely bugs, and should be fixed rather than hidden as exceptions.

The reason for the KNOWN_VIOLATIONS list is to be able to guard against introduction of new locale dependence by breaking the build if a new violation is introduced.

Thus the addition of an entry in KNOWN_VIOLATIONS should not be seen as an approval of the use of a locale dependent function, or a way to "hide" such use.

FWIW I consider all entries in KNOWN_VIOLATIONS to be potential bugs :) Ideally that list should empty.

@practicalswift At least a few of the ::to_string uses are not bugs, and should be locale-dependent... ;)

Is there an example locale to test with where std::to_string does not return the desired result? I couldn't find one.

@MarcoFalke

#include <cassert>
#include <clocale>
#include <string>

int main() {
  std::setlocale(LC_ALL, "vi_VN");
  assert(std::to_string(1.234567) == "1.234567");
}

$ ./loc
loc: loc.cpp:7: int main(): Assertion `std::to_string(1.234567) == "1.234567"' failed.
Aborted (core dumped)

Somewhat related: surprising isspace(…) in interesting locales.

I don't think we use std::to_string for floats, only for ints. So the current usage should be fine?

@MarcoFalke

Apart from formatting issues relying on the current locale might introduce threading gotchas. Note that concurrent calls to std::to_string from multiple threads may result in partial serialisation of calls :)

Personally I've given up on trying to divide unintentional locale dependence into the buckets "unsafe" and "safe": given the amount of gotchas I err on the side of caution and simply consider all unintentional locale dependence to be potentially unsafe :)

While waiting for the locale-independent std::to_chars (C++17) we could perhaps use a local ToChars(T)? To make things simple we could limit ourselves to the std::is_integral<T>::value case.

std::to_chars implementations:

• libstdc++
• libc++
• MSVC's STL

I don't think we use std::to_string for floats, only for ints.

I'm afraid that is not correct :(

I don't think we use std::to_string for floats, only for ints. So the current usage should be fine?

Only usage I could find of something that isn't an int is twice for double: https://github.com/bitcoin/bitcoin/blob/95ca6aeec7b8d9dbf39e3a036a5c238634ce3793/src/test/blockchain_tests.cpp#L33

While waiting for the locale-independent std::to_chars (C++17) we could perhaps use a local ToChars(T)? To make things simple we could limit ourselves to the std::is_integral<T>::value case.

Is this our only option? can we use this instead? https://github.com/bitcoin/bitcoin/blob/master/src/tinyformat.h#L1070 (the only caveat is that it will require us to provide the right fmt string, altough if that's a big deal we can easily write specialization wrappers)

While waiting for the locale-independent std::to_chars (C++17) we could perhaps use a local ToChars(T)? To make things simple we could limit ourselves to the std::is_integral<T>::value case.

Is this our only option? can we use this instead? https://github.com/bitcoin/bitcoin/blob/master/src/tinyformat.h#L1070 (the only caveat is that it will require us to provide the right fmt string, altough if that's a big deal we can easily write specialization wrappers)

my vote is on this, although I think this would be better and cleaner: https://github.com/fmtlib/fmt

Tinyformat is localized too, I believe. It uses the standard c++ stringstream interface.

Tinyformat is localized too, I believe. It uses the standard c++ stringstream interface.

:( I wasn't sure but couldn't find the word "locale" in the repo. sucks.

Why not using something along the lines of …

template <typename T>
std::string ToString(const T t) {
  std::ostringstream oss;
  oss.imbue(std::locale::classic());
  oss << t;
  return oss.str();
}

… ? :)