Replace uses of boost::trim* with locale-independent alternatives

Empact commented at 7:28 pm on February 12, 2020: member

TrimString is an existing alternative. ~I patterned TrimRight after it.~

Note TrimString ~and TrimRight~ uses " \f\n\r\t\v" as the pattern, which is consistent with the default behavior of std::isspace. https://en.cppreference.com/w/cpp/string/byte/isspace

Empact renamed this:
~~Replace uses of boost::trim* with non-locale-dependent alternatives~~
Replace uses of boost::trim* with locale-independent alternatives
on Feb 12, 2020

Empact commented at 7:38 pm on February 12, 2020: member

~Oops, should have checked outstanding PRs - #17760 covers this.~ See also #17760 (now closed)

practicalswift commented at 7:41 pm on February 12, 2020: contributor

Concept ACK

Thanks a lot for getting rid of the use of locale dependent functions! That is appreciated. Please don’t hesitate to take on some of the remaining KNOWN_VIOLATIONS in subsequent pull requests. I would be happy to review :)

in src/bitcoin-tx.cpp:774 in 7cf3e02096 outdated

770@@ -766,9 +771,7 @@ static std::string readStdin()
771     if (ferror(stdin))
772         throw std::runtime_error("error reading stdin");
773 
774-    boost::algorithm::trim_right(ret);
775-
776-    return ret;
777+    return TrimRight(ret);

practicalswift commented at 7:44 pm on February 12, 2020:

I think TrimString(…) is okay to use here. That would remove the need to add and thus maintain TrimRight(…).

Empact commented at 7:48 pm on February 12, 2020:

I’d be happy to do that but went with this initially as less likely to introduce behavior changes and perhaps easier to review.

Empact commented at 7:58 pm on February 12, 2020:

Replaced. 👍

Empact force-pushed on Feb 12, 2020

practicalswift commented at 7:48 pm on February 12, 2020: contributor

ACK 7cf3e02096d7991fc144ef05f5f6b7fecd475b8c modulo that the existing TrimString should be used in both places - that way TrimRight doesn’t need to be introduced and maintained :)

Empact force-pushed on Feb 12, 2020

Empact commented at 7:57 pm on February 12, 2020: member

Ok, took another look and it’s straightforward to see that the value goes only to DecodeHexTx which fails on non-hex digits, so I’ve switched that to TrimString as well.

DrahtBot added the label RPC/REST/ZMQ on Feb 12, 2020

DrahtBot added the label Tests on Feb 12, 2020

fanquake removed the label RPC/REST/ZMQ on Feb 12, 2020

fanquake removed the label Tests on Feb 12, 2020

fanquake added the label Refactoring on Feb 12, 2020

practicalswift commented at 9:51 pm on February 12, 2020: contributor

ACK fbc8195427cf98bf96eb76bafda39a4f38de3cd2

DrahtBot commented at 11:01 pm on February 12, 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:

#21727 (refactor: Move more stuff to blockstorage by MarcoFalke)
#20452 (util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) by practicalswift)

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.

Empact force-pushed on Feb 13, 2020

Empact commented at 0:14 am on February 13, 2020: member

Small fixup - used clang-format-diff.py to identify a whitespace inconsistency in the last commit:

 0diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
 1index c3e08106e..15b8d3215 100644
 2--- a/src/test/util_tests.cpp
 3+++ b/src/test/util_tests.cpp
 4@@ -155,7 +155,7 @@ BOOST_AUTO_TEST_CASE(util_TrimString)
 5     BOOST_CHECK_EQUAL(TrimString("\t \n foo \n\tbar\t \n ", "fobar"), "\t \n foo \n\tbar\t \n ");
 6     BOOST_CHECK_EQUAL(TrimString("foo bar"), "foo bar");
 7     BOOST_CHECK_EQUAL(TrimString("foo bar", "fobar"), " ");
 8-    BOOST_CHECK_EQUAL(TrimString(std::string( "\0 foo \0 ", 8)), std::string("\0 foo \0", 7));
 9+    BOOST_CHECK_EQUAL(TrimString(std::string("\0 foo \0 ", 8)), std::string("\0 foo \0", 7));
10     BOOST_CHECK_EQUAL(TrimString(std::string(" foo ", 5)), std::string("foo", 3));
11     BOOST_CHECK_EQUAL(TrimString(std::string("\t\t\0\0\n\n", 6)), std::string("\0\0", 2));
12     BOOST_CHECK_EQUAL(TrimString(std::string("\x05\x04\x03\x02\x01\x00", 6)), std::string("\x05\x04\x03\x02\x01\x00", 6));

practicalswift commented at 0:21 am on February 13, 2020: contributor

ACK 44f18a552e51cdb103845b592b87dbab571f8308

practicalswift commented at 1:41 pm on March 6, 2020: contributor

Anyone willing to review @Empact’s excellent PR? Would be really nice to move forward with this one and plug another source of locale dependency.

l2a5b1 commented at 7:41 pm on March 9, 2020: contributor

ACK 44f18a5 - nice! Next boost::split 🙄

DrahtBot added the label Needs rebase on Mar 25, 2020

Empact force-pushed on Mar 25, 2020

practicalswift commented at 9:30 pm on March 25, 2020: contributor

re-ACK 4c1abe60467c1adfe0230f4bee56202150c0b134

DrahtBot removed the label Needs rebase on Mar 25, 2020

Replace use of boost::trim use with locale-independent TrimString 3d0859ec74

Replace use of boost::trim_right with locale-independent TrimString

Note the only use of readStdin is fed to DecodeHexTx, which fails in
IsHex on non-hex characters as recorded in p_util_hexdigit.

5c0c94160f

tests: Add TrimString(...) tests 12c0546952

Empact force-pushed on May 11, 2020

practicalswift commented at 7:45 pm on November 22, 2020: contributor

re-ACK 12c054695201db175800e50946a85bbaa8185b9b

Nice to see the list of places where we rely on locale dependent functions shrink :)

DrahtBot commented at 3:51 pm on May 5, 2021: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on May 5, 2021

fanquake commented at 9:57 am on August 17, 2021: member

@Empact Any chance you’d like to rebase this? Otherwise I can pick it up.

fanquake commented at 5:27 am on September 2, 2021: member

I’ve rebased this in #22859.

fanquake closed this on Sep 2, 2021

fanquake referenced this in commit 6490a3ef6c on Sep 5, 2021

DrahtBot locked this on Sep 2, 2022

Replace uses of boost::trim* with locale-independent alternatives #18130

Conflicts