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

pull Empact wants to merge 1 commits into bitcoin:master from Empact:2020-02-to-string changing 19 files +71 −58
  1. 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.~

  2. fanquake added the label Refactoring on Feb 13, 2020
  3. Empact force-pushed on Feb 13, 2020
  4. Empact force-pushed on Feb 13, 2020
  5. 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~

  6. DrahtBot commented at 8:46 AM on February 13, 2020: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

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

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

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

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

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

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

  13. emilengler commented at 5:36 PM on February 19, 2020: contributor

    Concept ACK

  14. promag commented at 7:54 AM on February 21, 2020: member

    Concept ACK.

  15. 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)).

  16. Empact force-pushed on Mar 9, 2020
  17. Empact force-pushed on Mar 9, 2020
  18. Empact commented at 5:49 PM on March 9, 2020: member

    Rebased and incorporated your feedback, @practicalswift

  19. practicalswift commented at 6:46 PM on March 9, 2020: contributor

    ACK 97eefd7c25f03e898071171eacc45ac5fb5ada6b

  20. DrahtBot added the label Needs rebase on Mar 11, 2020
  21. Empact force-pushed on Mar 11, 2020
  22. DrahtBot removed the label Needs rebase on Mar 11, 2020
  23. Empact commented at 1:24 AM on March 13, 2020: member

    Rebased

  24. 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: 
     * Locale-independent version of std::to_string
  25. 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.

  26. sipa commented at 2:11 AM on March 13, 2020: member

    ACK 0add74b0b5e11124612e6d07b3f68266a4ace66e, minor nits

  27. practicalswift commented at 8:07 AM on March 13, 2020: contributor

    ACK 0add74b0b5e11124612e6d07b3f68266a4ace66e

  28. Replace std::to_string with locale-independent alternative d056df033a
  29. Empact force-pushed on Mar 14, 2020
  30. Empact commented at 7:24 PM on March 14, 2020: member

    Addressed nits.

  31. 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! :)

  32. laanwj commented at 7:11 PM on March 25, 2020: member

    ACK d056df033a1e88554f7cc39dd709a87b17cb49df

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

     // Added for Bitcoin Core
 template<typename... Args>
 std::string format(const std::string &fmt, const Args&... args)
 {
     std::ostringstream oss;
+    oss.imbue(std::locale::classic());
     format(oss, fmt.c_str(), args...);
     return oss.str();
 }
  33. laanwj merged this on Mar 25, 2020
  34. laanwj closed this on Mar 25, 2020
  35. 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! :)

  36. sidhujag referenced this in commit 2ca9e861e3 on Mar 28, 2020
  37. Fabcien referenced this in commit 47a5b8aad9 on Jan 12, 2021
  38. DrahtBot locked this on Feb 15, 2022
Contributors
EmpactDrahtBotpracticalswiftemilenglerpromagsipaajtownslaanwj
Labels
Refactoring
Linked (view graph)
#17866 check std::to_string usage #18432 util: Make our stringstream usage locale independent#21422 Add feerate histogram to getmempoolinfo
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-30 00:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me