Replace uses of boost::trim* with locale-independent alternatives (#18130 rebased) #22859

pull fanquake wants to merge 3 commits into bitcoin:master from fanquake:18130_rebased changing 4 files +20 −8

fanquake commented at 5:26 AM on September 2, 2021: member

This is #18130 rebased.

TrimString is an existing alternative.

Note TrimString uses " \f\n\r\t\v" as the pattern, which is consistent with the default behavior of std::isspace. See: https://en.cppreference.com/w/cpp/string/byte/isspace
Replace use of boost::trim use with locale-independent TrimString 93551862a1

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.

4bf18b089e

tests: Add TrimString(...) tests 696c76d660
fanquake added the label Refactoring on Sep 2, 2021
fanquake commented at 5:27 AM on September 2, 2021: member

cc @practicalswift @l2a5b1
jonatack commented at 4:52 PM on September 3, 2021: member

Concept ACK
jb55 commented at 9:46 PM on September 3, 2021: member

utACK 696c76d6604c9c4faddfc4b6684e2788bb577ba6
practicalswift commented at 1:26 PM on September 4, 2021: contributor

Very happy to see a reduction in our locale dependency!

I think we'll be able to have an empty KNOWN_VIOLATIONS list in test/lint/lint-locale-dependence.sh by the end of the year. That would be awesome! :)

ACK 696c76d6604c9c4faddfc4b6684e2788bb577ba6

in src/httprpc.cpp:135 in 696c76d660

 130 | @@ -130,8 +131,7 @@ static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUserna
 131 |          return false;
 132 |      if (strAuth.substr(0, 6) != "Basic ")
 133 |          return false;
 134 | -    std::string strUserPass64 = strAuth.substr(6);
 135 | -    boost::trim(strUserPass64);
 136 | +    std::string strUserPass64 = TrimString(strAuth.substr(6));
 137 |      std::string strUserPass = DecodeBase64(strUserPass64);

jonatack commented at 2:47 PM on September 4, 2021:

9355186 nit, these can be const

-    std::string strUserPass64 = TrimString(strAuth.substr(6));
-    std::string strUserPass = DecodeBase64(strUserPass64);
+    const std::string strUserPass64 = TrimString(strAuth.substr(6));
+    const std::string strUserPass = DecodeBase64(strUserPass64);

jonatack commented at 2:48 PM on September 4, 2021: member

ACK 696c76d6604c9c4faddfc4b6684e2788bb577ba6
theStack approved
theStack commented at 10:49 PM on September 4, 2021: member

Code-review ACK 696c76d6604c9c4faddfc4b6684e2788bb577ba6

Happy to also re-ACK if Jon's suggestion of declaring strUserPass{64} as const (https://github.com/bitcoin/bitcoin/pull/22859#discussion_r702292854) is applied.
fanquake merged this on Sep 5, 2021
fanquake closed this on Sep 5, 2021
fanquake deleted the branch on Sep 5, 2021
sidhujag referenced this in commit 7265550bd6 on Sep 7, 2021
in src/bitcoin-tx.cpp:775 in 696c76d660
```
 771 | @@ -772,9 +772,7 @@ static std::string readStdin()
 772 |      if (ferror(stdin))
 773 |          throw std::runtime_error("error reading stdin");
 774 |  
 775 | -    boost::algorithm::trim_right(ret);
 776 | -
 777 | -    return ret;
 778 | +    return TrimString(ret);
```
luke-jr commented at 4:36 PM on September 20, 2021:

This SHOULD be a locale trim!

sipa commented at 4:37 PM on September 20, 2021:

Why? It isn't trying to read native-language text.

luke-jr commented at 5:17 PM on September 20, 2021:

It's interacting with the CLI, which is expected to behave in the user's configured locale.

sipa commented at 7:59 PM on September 20, 2021:

When processing text, or other locale-formatted things like dates/..., I can see that. But this is just accepting hex-encoded tx data?
DrahtBot locked this on Oct 30, 2022

Contributors

Labels

Linked (view graph)

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