check std::to_string usage

fanquake commented at 6:27 am on January 4, 2020: member

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

fanquake added the label good first issue on Jan 4, 2020

practicalswift commented at 10:38 am on January 4, 2020: contributor

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.

luke-jr commented at 2:35 pm on January 4, 2020: member

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

fanquake deleted a comment on Jan 19, 2020

MarcoFalke commented at 0:11 am on January 20, 2020: member

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

practicalswift commented at 12:53 pm on January 20, 2020: contributor

@MarcoFalke

0#include <cassert>
1#include <clocale>
2#include <string>
3
4int main() {
5  std::setlocale(LC_ALL, "vi_VN");
6  assert(std::to_string(1.234567) == "1.234567");
7}

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

MarcoFalke commented at 1:38 pm on January 20, 2020: member

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

practicalswift commented at 1:58 pm on January 20, 2020: contributor

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

practicalswift commented at 2:24 pm on January 20, 2020: contributor

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:

practicalswift commented at 4:30 pm on January 23, 2020: contributor

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

I’m afraid that is not correct :(

sanjaykdragon commented at 3:21 pm on January 30, 2020: contributor

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

elichai commented at 3:16 pm on February 3, 2020: contributor

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)

sanjaykdragon commented at 6:57 pm on February 3, 2020: contributor

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

sipa commented at 7:00 pm on February 3, 2020: member

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

elichai commented at 7:05 pm on February 3, 2020: contributor

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.

practicalswift commented at 6:40 pm on February 10, 2020: contributor

Why not using something along the lines of …

0template <typename T>
1std::string ToString(const T t) {
2  std::ostringstream oss;
3  oss.imbue(std::locale::classic());
4  oss << t;
5  return oss.str();
6}

… ? :)

laanwj referenced this in commit 2e97d80017 on Mar 25, 2020

sidhujag referenced this in commit 2ca9e861e3 on Mar 28, 2020

MarcoFalke closed this on Apr 27, 2020

DrahtBot locked this on Feb 15, 2022

check std::to_string usage #17866