refactor: introduce single-separator split helper (boost::split replacement)

[theStack]

This PR adds a simple string split helper SplitString that takes use of the spanparsing Split function that was first introduced in #13697 (commit fe8a7dc). This enables to replace most calls to boost::split, in the cases where only a single separator character is used. Note that while previous attempts to replace boost::split were controversial (e.g. #13751), this one has a trivial implementation: it merely uses an internal helper (that is unit tested and in regular use with output descriptiors) and converts its result from spans to strings. As a drawback though, not all boost::split instances can be tackled.

As a possible optimization, one could return a vector of std::string_views (available since C++17) instead of strings, to avoid copies. This would need more carefulness on the caller sites though, to avoid potential lifetime issues, and it's probably not worth it, considering that none of the places where strings are split are really performance-critical.

[maflcko]

Seems ok to use the already existing split function.

[theStack]

Force-pushed with changes as suggested by MarcoFalke (#22953 (comment)).

Pinging @practicalswift as someone who has always shown interest in potential boost replacements and string processing helpers, with lots of contributions in that area. Maybe you feel like taking a look :)

[kiminuo]

I would still add a unit test even though there are tests for the internal helper. Maybe it's a little bit over the top so feel free to ignore my suggestion.

(Something like this possibly: kiminuo@ac6dd5a)

[maflcko]

Any reason the tests aren't fixed up as well?

src/test/fuzz/script_assets_test_minimizer.cpp:    boost::algorithm::split(words, str, boost::algorithm::is_any_of(","));

Also, if you add a unit test, might as extend the string fuzz target.

[kiminuo]

There non-tests occurrences too:

src\rpc\server.cpp:
  410          std::vector<std::string> vargNames;
  411:         boost::algorithm::split(vargNames, argNamePattern, boost::algorithm::is_any_of("|"));
  412          auto fr = argsIn.end();

src\test\transaction_tests.cpp:
  72      std::vector<std::string> words;
  73:     boost::algorithm::split(words, strFlags, boost::algorithm::is_any_of(","));
  74  

src\test\fuzz\script_assets_test_minimizer.cpp:
  133      std::vector<std::string> words;
  134:     boost::algorithm::split(words, str, boost::algorithm::is_any_of(","));
  135  

If I'm not missing something.

edit: Ah, I'm not aware what the difference is between boost::algorithm::split and boost::split. Seems like none?

[theStack]

I would still add a unit test even though there are tests for the internal helper. Maybe it's a little bit over the top so feel free to ignore my suggestion.

I agree that it's a good idea to add a unit test.

(Something like this possibly: kiminuo@ac6dd5a)

Thanks! As quick feedback, can you change the commit subject to be more verbose (e.g. "test: add unit tests for SplitString helper") and put the test into src/test/util_tests.cpp, instead of creating a new file? I'm happy to either add your commit to this PR, or review your PR if you decide to open your own, based on this one.

Any reason the tests aren't fixed up as well?
...

There non-tests occurrences too:
...

Oh, I only greped for boost::split, not being aware that there is also boost::algorithm::split, which seems to be a synonym. Force-pushed with changes to tackle also the remaining occurences. Also found that there were some unnecessary boost includes in src/torcontrol.cpp that I forgot to remove in the previous push.

[kiminuo]

(Something like this possibly: kiminuo@ac6dd5a)

Thanks! As quick feedback, can you change the commit subject to be more verbose (e.g. "test: add unit tests for SplitString helper") and put the test into src/test/util_tests.cpp, instead of creating a new file? I'm happy to either add your commit to this PR, or review your PR if you decide to open your own, based on this one.

Modified here: kiminuo@4516727. Anyway, feel free to modify the commit in whatever way you like :)

[maflcko]

The fuzz test I mentioned: (might not compile)

diff --git a/src/test/fuzz/string.cpp b/src/test/fuzz/string.cpp
index 0c1b45b86c..e5a09c0013 100644
--- a/src/test/fuzz/string.cpp
+++ b/src/test/fuzz/string.cpp
@@ -127,6 +127,7 @@ FUZZ_TARGET(string)
         int64_t amount_out;
         (void)ParseFixedPoint(random_string_1, fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 1024), &amount_out);
     }
+    (void)SplitString(random_string_1, fuzzed_data_provider.ConsumeIntegral<char>());
     {
         (void)Untranslated(random_string_1);
         const bilingual_str bs1{random_string_1, random_string_2};

[theStack]

The fuzz test I mentioned: (might not compile)

Oh, I didn't see that in your previous comment. It compiles, I added another commit 👌

[fanquake]

Concept ACK - this is low-overhead for dropping more Boost dependence. Surprised we can't also nuke anything from test/lint/lint-includes.sh

[laanwj]

Concept ACK. I don't think this boost use is very bad (it's a header-only library) but if we can replace it with functionality we already have internally, who not.

Concept ACK - this is low-overhead for dropping more Boost dependence. Surprised we can't also nuke anything from test/lint/lint-includes.sh

Well, split is still used in some places (httprpc.cpp for example), this only replaces the single-separator case. It will take some more work to get rid of all boost/algorithm/string.hpp use, if that's worthwhile.

[theStack]

Force-pushed the alternative variant as suggested by @martinus, changing spanparsing::Split to a templated function and defining an alias SplitString as suggested by @MarcoFalke (see #22953 (comment) and #22953 (comment)).

[maflcko]

Not a fan of forcing devs to remember to construct a std::string manually each time before calling the function.

[maflcko]

It seems dangerous to offer a function that silently does the wrong thing when passed the "wrong" type

[theStack]

Agree. This is fixed now by using an explicit interface instead of an alias, as suggested by martinus in #22953 (comment).

.

[martinus]

Windows build error seems unrelated, in wallet_address_types.py:

OSError: [WinError 10048] Only one usage of each socket address (protocol/network address/port) is normally permitted

[hebasto]

See #18548

On Thu, 23 Sep 2021 at 08:54, Martin Leitner-Ankerl < ***@***.***> wrote: Windows build error seems unrelated, in wallet_address_types.py: OSError: [WinError 10048] Only one usage of each socket address (protocol/network address/port) is normally permitted — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#22953 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AH3PXPQDTK3VEUUU4R2KEY3UDK6JVANCNFSM5D246WWQ> .

--

…

-- Hennadii Stepanov

[theStack]

Rebased on master.

[theStack]

Rebased on master again (there was a conflict in src/rest.cpp after #24098 has been merged).

[fanquake]

@martinus want to take another look here?

[martinus]

Code review ACK a62e844. Ran all tests. I also like that with boost::split it was not obvious that the resulting container was cleared, and with SplitString API that's obvious.

After that PR there is only one usage of boost::split left in core_read.cpp, that would be a nice followup PR 🙂