Use functions guaranteed to be locale independent (IsDigit, ToLower) in {Format,Parse}Money(...), uint256::SetHex(...), etc. Remove the use of locale dependent boost::is_space(...) #14599

pull practicalswift wants to merge 5 commits into bitcoin:master from practicalswift:no-more-locale changing 6 files +11 −18
  1. practicalswift commented at 8:50 AM on October 29, 2018: contributor
    • Use ToLower(...) instead of std::tolower. std::tolower is locale dependent.
    • Use IsDigit(...) instead of std::isdigit. Some implementations (e.g. Microsoft in 1252 codepage) may classify single-byte characters other than [0-9] as digits.
    • Update KNOWN_VIOLATIONS: Remove fixed violations.
    • Replace use of locale dependent Boost trim (boost::trim) with locale independent TrimString.
    • Use IsSpace(...) instead of boost::is_space
  2. ken2812221 commented at 9:08 AM on October 29, 2018: contributor

    ~utACK 25fc82ccf0d48b5c4d35d47b657c4ba5945c8d3c~ require #include <utilstrencoding.h> in wallet disabled configure.

    I wonder that if this linter has a Good job! ... locale dependency removed error.

  3. fanquake added the label Refactoring on Oct 29, 2018
  4. practicalswift force-pushed on Oct 29, 2018
  5. practicalswift force-pushed on Oct 29, 2018
  6. practicalswift commented at 10:19 AM on October 29, 2018: contributor

    @ken2812221 Thanks for the quick review! Please re-review :-)

    The locale linter doesn't have the positive "Good job!" message (in the case a dependency in KNOWN_VIOLATIONS is removed). That would have been nice though :-)

  7. practicalswift force-pushed on Oct 29, 2018
  8. in src/uint256.cpp:36 in 124fdab04a outdated 
      32 | @@ -33,7 +33,7 @@ void base_blob<BITS>::SetHex(const char* psz)
  33 |          psz++;
  34 |  
  35 |      // skip 0x
  36 | -    if (psz[0] == '0' && tolower(psz[1]) == 'x')
  37 | +    if (psz[0] == '0' && ToLower(psz[1]) == 'x')
    practicalswift commented at 10:53 PM on October 29, 2018:

    Self-review:

    Implicit conversion from char to unsigned char here. Should be handled explicitly?

  9. practicalswift force-pushed on Oct 30, 2018
  10. practicalswift force-pushed on Oct 30, 2018
  11. practicalswift force-pushed on Oct 30, 2018
  12. practicalswift renamed this:
    Use functions guaranteed to be locale independent (IsDigit, ToLower) in {Format,Parse}Money(...), uint256::SetHex(...), etc.
    Use functions guaranteed to be locale independent (IsDigit, ToLower) in {Format,Parse}Money(...), uint256::SetHex(...), etc. Remove the use of locale dependent boost::trim(...).
    on Oct 30, 2018
  13. practicalswift commented at 4:00 PM on October 30, 2018: contributor

    Added commit:

    • Replace use of locale dependent Boost trim (boost::trim) with locale independent TrimString.
  14. DrahtBot commented at 4:35 PM on October 30, 2018: 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:

    • #13751 (Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency by 251Labs)

    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.

  15. in src/bitcoin-tx.cpp:770 in c49621b5ad outdated 
     766 | @@ -767,7 +767,7 @@ static std::string readStdin()
 767 |      if (ferror(stdin))
 768 |          throw std::runtime_error("error reading stdin");
 769 |  
 770 | -    boost::algorithm::trim_right(ret);
 771 | +    ret = TrimString(ret, " \f\n\r\t\v");
    laanwj commented at 1:17 PM on November 1, 2018:

    this changes functionality (trim right to general trim), I don't think you should do that at the same time

    practicalswift commented at 2:48 PM on November 1, 2018:

    @laanwj We don't have any right-trim function and from what I can see there is no harm in also left-trimming in this case. If you prefer I can add a right-trim function, should I do that or is it okay to left-trim?

    practicalswift commented at 8:51 AM on November 2, 2018:

    @jgarzik You added this function in fb14452c6c - do you think it is OK to trim (instead of only right-trimming) this string in order to avoid having to add a project local right-trim? :-)

    MarcoFalke commented at 4:00 PM on November 6, 2018:

    This conflicts with another pull request and needs probably more review than the other changes. Mind to leave it out for now and create a pull request later for the trim change?

  16. laanwj commented at 1:22 PM on November 1, 2018: member

    utACK apart from the rtrim to trim change

  17. DrahtBot added the label Needs rebase on Nov 5, 2018
  18. practicalswift force-pushed on Nov 5, 2018
  19. practicalswift commented at 12:41 PM on November 5, 2018: contributor

    Rebased!

  20. DrahtBot removed the label Needs rebase on Nov 5, 2018
  21. practicalswift force-pushed on Nov 5, 2018
  22. practicalswift force-pushed on Nov 5, 2018
  23. practicalswift commented at 11:02 PM on November 5, 2018: contributor

    Added another commit: 2f3ae54ac6dd2fe36c2f469d3b96b2a7bfb5ce90 ("Use IsSpace(...) instead of boost::is_space")

    Please review :-)

  24. practicalswift renamed this:
    Use functions guaranteed to be locale independent (IsDigit, ToLower) in {Format,Parse}Money(...), uint256::SetHex(...), etc. Remove the use of locale dependent boost::trim(...).
    Use functions guaranteed to be locale independent (IsDigit, ToLower) in {Format,Parse}Money(...), uint256::SetHex(...), etc. Remove the use of locale dependent boost::trim(...) and boost::is_space(...)
    on Nov 5, 2018
  25. Use IsDigit(...) instead of std::isdigit e70cc8983c
  26. Use ToLower(...) instead of std::tolower c5fd143edb
  27. Use IsSpace(...) instead of boost::is_space 587924f000
  28. Update KNOWN_VIOLATIONS: Remove fixed violations 7c9f790761
  29. practicalswift force-pushed on Nov 6, 2018
  30. practicalswift commented at 4:39 PM on November 6, 2018: contributor

    @MarcoFalke @laanwj I've now excluded the trim change to make reviewing easier. Please re-review :-)

  31. practicalswift renamed this:
    Use functions guaranteed to be locale independent (IsDigit, ToLower) in {Format,Parse}Money(...), uint256::SetHex(...), etc. Remove the use of locale dependent boost::trim(...) and boost::is_space(...)
    Use functions guaranteed to be locale independent (IsDigit, ToLower) in {Format,Parse}Money(...), uint256::SetHex(...), etc. Remove the use of locale dependent boost::is_space(...)
    on Nov 6, 2018
  32. MarcoFalke commented at 9:09 PM on November 8, 2018: member

    utACK 7c9f7907615ff9c10a56ede5a8e47c91cb20fe3b

  33. in src/test/getarg_tests.cpp:20 in 7c9f790761 outdated 
      16 | @@ -17,7 +17,7 @@ static void ResetArgs(const std::string& strArg)
  17 |  {
  18 |      std::vector<std::string> vecArg;
  19 |      if (strArg.size())
  20 | -      boost::split(vecArg, strArg, boost::is_space(), boost::token_compress_on);
  21 | +      boost::split(vecArg, strArg, IsSpace, boost::token_compress_on);
    hebasto commented at 11:00 AM on December 6, 2018:

    Could add

    #include <util/strencodings.h>

    to the headers?

  34. hebasto changes_requested
  35. Include util/strencodings.h which is required for IsSpace(...) 8931a95bec
  36. hebasto commented at 11:15 AM on December 6, 2018: member

    utACK 8931a95beca2b959c7ee73b154ce8a69acbe8599

  37. MarcoFalke commented at 6:48 PM on December 6, 2018: member

    re-utACK 8931a95beca2b959c7ee73b154ce8a69acbe8599

  38. MarcoFalke added this to the milestone 0.18.0 on Dec 6, 2018
  39. practicalswift commented at 9:03 PM on December 28, 2018: contributor

    @laanwj @ken2812221 Would you mind re-reviewing? :-)

  40. in src/uint256.cpp:36 in 8931a95bec 
      32 | @@ -33,7 +33,7 @@ void base_blob<BITS>::SetHex(const char* psz)
  33 |          psz++;
  34 |  
  35 |      // skip 0x
  36 | -    if (psz[0] == '0' && tolower(psz[1]) == 'x')
  37 | +    if (psz[0] == '0' && ToLower((unsigned char)psz[1]) == 'x')
    laanwj commented at 5:16 PM on January 9, 2019:

    It's kind of strange for a ToLower function to take an unsigned char (because std::strings index as char), but that's not introduced in this PR.

  41. laanwj commented at 5:17 PM on January 9, 2019: member

    re-utACK 8931a95beca2b959c7ee73b154ce8a69acbe8599

  42. laanwj merged this on Jan 9, 2019
  43. laanwj closed this on Jan 9, 2019
  44. laanwj referenced this in commit 62f3977f60 on Jan 9, 2019
  45. deadalnix referenced this in commit 52379b02de on Jun 2, 2020
  46. ftrader referenced this in commit 8f46164d25 on Aug 17, 2020
  47. practicalswift deleted the branch on Apr 10, 2021
  48. Munkybooty referenced this in commit f5648502bf on Aug 20, 2021
  49. Munkybooty referenced this in commit 4db6b8d9cf on Aug 20, 2021
  50. Munkybooty referenced this in commit fda30a9391 on Aug 20, 2021
  51. dzutto referenced this in commit fa67372efe on Sep 9, 2021
  52. vijaydasmp referenced this in commit 7a656e185f on Sep 9, 2021
  53. UdjinM6 referenced this in commit be2bd03370 on Sep 9, 2021
  54. malbit referenced this in commit 00a1454ed0 on Apr 10, 2022
  55. gades referenced this in commit 0c97fb8b42 on May 9, 2022
  56. DrahtBot locked this on Aug 18, 2022
Contributors
practicalswiftken2812221DrahtBotlaanwjMarcoFalkehebasto
Labels
Refactoring

Milestone
0.18.0

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-16 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me