Repository: bitcoin/bitcoin
File: src/util/string.h (C/C++)

Code before:
template <typename T = Span<const char>>
std::vector<T> Split(const Span<const char>& sp, char sep)
{
    return Split<T>(sp, std::string_view{&sep, 1});
}

[[nodiscard]] inline std::vector<std::string> SplitString(std::string_view str, char sep)
{
    return Split<std::string>(str, sep);
}

[[nodiscard]] inline std::vector<std::string> SplitString(std::string_view str, std::string_view separators)
{
    return Split<std::string>(str, separators);
}

[[nodiscard]] inline std::string_view TrimStringView(std::string_view str, std::string_view pattern = " \f\n\r\t\v")
{
    std::string::size_type front = str.find_first_not_of(pattern);
    if (front == std::string::npos) {
        return {};
    }
    std::string::size_type end = str.find_last_not_of(pattern);
    return str.substr(front, end - front + 1);
}

[[nodiscard]] inline std::string TrimString(std::string_view str, std::string_view pattern = " \f\n\r\t\v")
{
    return std::string(TrimStringView(str, pattern));
}

[[nodiscard]] inline std::string_view RemoveSuffixView(std::string_view str, std::string_view suffix)
{
    if (str.ends_with(suffix)) {
        return str.substr(0, str.size() - suffix.size());
    }
    return str;
}

[[nodiscard]] inline std::string_view RemovePrefixView(std::string_view str, std::string_view prefix)
{
    if (str.substr(0, prefix.size()) == prefix) {
        return str.substr(prefix.size());
    }
    return str;
}

[[nodiscard]] inline std::string RemovePrefix(std::string_view str, std::string_view prefix)
{
    return std::string(RemovePrefixView(str, prefix));
}

Review comment (bug):
I think having a separate `CountNumberFormatSpecifiers` would have made testing this function a bit easier and with better error feedback, while simultaneously also reducing template instantiation. Neither are overwhelmingly strong arguments, but I don't think there are really any downsides either?

E.g. sample, pretty rough (but passes tests) diff:

<details>
<summary>git diff on faa32adbcf</summary>

```diff
diff --git a/src/test/util_string_tests.cpp b/src/test/util_string_tests.cpp
index 1e041f37c6..43b63751d9 100644
--- a/src/test/util_string_tests.cpp
+++ b/src/test/util_string_tests.cpp
@@ -7,51 +7,58 @@
 #include <boost/test/unit_test.hpp>
 
 using namespace util;
+using namespace util::detail;
 
 BOOST_AUTO_TEST_SUITE(util_string_tests)
 
 BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
 {
-    // Compile-time sanity checks
-    static_assert([] {
-        ConstevalFormatString<0>::Detail_CheckNumFormatSpecifiers("");
-        ConstevalFormatString<0>::Detail_CheckNumFormatSpecifiers("%%");
-        ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%s");
-        ConstevalFormatString<0>::Detail_CheckNumFormatSpecifiers("%%s");
-        ConstevalFormatString<0>::Detail_CheckNumFormatSpecifiers("s%%");
-        ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%%%s");
-        ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%s%%");
-        ConstevalFormatString<0>::Detail_CheckNumFormatSpecifiers(" 1$s");
-        ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%1$s");
-        ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%1$s%1$s");
-        ConstevalFormatString<2>::Detail_CheckNumFormatSpecifiers("%2$s");
-        ConstevalFormatString<2>::Detail_CheckNumFormatSpecifiers("%2$s 4$s %2$s");
-        ConstevalFormatString<129>::Detail_CheckNumFormatSpecifiers("%129$s 999$s %2$s");
-        return true; // All checks above compiled and passed
-    }());
+    BOOST_CHECK_EQUAL(0, CountNumFormatSpecifiers(""));
+    BOOST_CHECK_EQUAL(0, CountNumFormatSpecifiers("%%"));
+    BOOST_CHECK_EQUAL(1, CountNumFormatSpecifiers("%s"));
+    BOOST_CHECK_EQUAL(0, CountNumFormatSpecifiers("%%s"));
+    BOOST_CHECK_EQUAL(0, CountNumFormatSpecifiers("s%%"));
+    BOOST_CHECK_EQUAL(1, CountNumFormatSpecifiers("%%%s"));
+    BOOST_CHECK_EQUAL(1, CountNumFormatSpecifiers("%s%%"));
+    BOOST_CHECK_EQUAL(0, CountNumFormatSpecifiers(" 1$s"));
+    BOOST_CHECK_EQUAL(1, CountNumFormatSpecifiers("%1$s"));
+    BOOST_CHECK_EQUAL(1, CountNumFormatSpecifiers("%1$s%1$s"));
+    BOOST_CHECK_EQUAL(2, CountNumFormatSpecifiers("%2$s"));
+    BOOST_CHECK_EQUAL(2, CountNumFormatSpecifiers("%2$s 4$s %2$s"));
+    BOOST_CHECK_EQUAL(129, CountNumFormatSpecifiers("%129$s 999$s %2$s"));
+
+    // existing "tests" in run-lint-format-strings.py parse_string_content
+    BOOST_CHECK_EQUAL(0, CountNumFormatSpecifiers("foo bar foo"));
+    BOOST_CHECK_EQUAL(1, CountNumFormatSpecifiers("foo %d bar foo"));
+    BOOST_CHECK_EQUAL(2, CountNumFormatSpecifiers("foo %d bar %i foo"));
+    BOOST_CHECK_EQUAL(2, CountNumFormatSpecifiers("foo %d bar %i foo %% foo"));
+    BOOST_CHECK_EQUAL(3, CountNumFormatSpecifiers("foo %d bar %i foo %% foo %d foo"));
+    // BOOST_CHECK_EQUAL(4, CountNumFormatSpecifiers("foo %d bar %i foo %% foo %*d foo"));
+    BOOST_CHECK_EQUAL(5, CountNumFormatSpecifiers("foo %5$d"));
+    // BOOST_CHECK_EQUAL(7, CountNumFormatSpecifiers("foo %5$*7$d"));
 
     // Negative checks at runtime
     using ErrType = const char*;
 
     auto check_mix{[](const ErrType& str) { return std::string_view{str} == "Format specifiers must be all positional or all non-positional!"; }};
-    BOOST_CHECK_EXCEPTION(ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%s%1$s"), ErrType, check_mix);
+    BOOST_CHECK_EXCEPTION(CountNumFormatSpecifiers("%s%1$s"), ErrType, check_mix);
 
     auto check_num_spec{[](const ErrType& str) { return std::string_view{str} == "Format specifier count must match the argument count!"; }};
-    BOOST_CHECK_EXCEPTION(ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers(""), ErrType, check_num_spec);
-    BOOST_CHECK_EXCEPTION(ConstevalFormatString<0>::Detail_CheckNumFormatSpecifiers("%s"), ErrType, check_num_spec);
-    BOOST_CHECK_EXCEPTION(ConstevalFormatString<2>::Detail_CheckNumFormatSpecifiers("%s"), ErrType, check_num_spec);
-    BOOST_CHECK_EXCEPTION(ConstevalFormatString<0>::Detail_CheckNumFormatSpecifiers("%1$s"), ErrType, check_num_spec);
-    BOOST_CHECK_EXCEPTION(ConstevalFormatString<2>::Detail_CheckNumFormatSpecifiers("%1$s"), ErrType, check_num_spec);
+    BOOST_CHECK_EXCEPTION(ConstevalFormatString<1>::CheckNumFormatSpecifiers(""), ErrType, check_num_spec);
+    BOOST_CHECK_EXCEPTION(ConstevalFormatString<0>::CheckNumFormatSpecifiers("%s"), ErrType, check_num_spec);
+    BOOST_CHECK_EXCEPTION(ConstevalFormatString<2>::CheckNumFormatSpecifiers("%s"), ErrType, check_num_spec);
+    BOOST_CHECK_EXCEPTION(ConstevalFormatString<0>::CheckNumFormatSpecifiers("%1$s"), ErrType, check_num_spec);
+    BOOST_CHECK_EXCEPTION(ConstevalFormatString<2>::CheckNumFormatSpecifiers("%1$s"), ErrType, check_num_spec);
 
     auto check_special{[](const ErrType& str) { return std::string_view{str} == "Invalid format specifier!"; }};
-    BOOST_CHECK_EXCEPTION(ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%_"), ErrType, check_special);
-    BOOST_CHECK_EXCEPTION(ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%\n"), ErrType, check_special);
+    BOOST_CHECK_EXCEPTION(CountNumFormatSpecifiers("%_"), ErrType, check_special);
+    BOOST_CHECK_EXCEPTION(CountNumFormatSpecifiers("%\n"), ErrType, check_special);
     // The "special" check also excludes "0", which would be out-of-range in tinyformat
-    BOOST_CHECK_EXCEPTION(ConstevalFormatString<0>::Detail_CheckNumFormatSpecifiers("%0$s"), ErrType, check_special);
+    BOOST_CHECK_EXCEPTION(CountNumFormatSpecifiers("%0$s"), ErrType, check_special);
 
     auto check_special_pos{[](const ErrType& str) { return std::string_view{str} == "Invalid positional format specifier!"; }};
-    BOOST_CHECK_EXCEPTION(ConstevalFormatString<0>::Detail_CheckNumFormatSpecifiers("%1$"), ErrType, check_special_pos);
-    BOOST_CHECK_EXCEPTION(ConstevalFormatString<0>::Detail_CheckNumFormatSpecifiers("%1$_"), ErrType, check_special_pos);
+    BOOST_CHECK_EXCEPTION(CountNumFormatSpecifiers("%1$"), ErrType, check_special_pos);
+    BOOST_CHECK_EXCEPTION(CountNumFormatSpecifiers("%1$_"), ErrType, check_special_pos);
 }
 
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 1624fb8b5b..ceb2e9ad54 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -154,18 +154,18 @@ BOOST_AUTO_TEST_CASE(parse_hex)
 
     // Basic test vector
     std::vector<unsigned char> expected(std::begin(HEX_PARSE_OUTPUT), std::end(HEX_PARSE_OUTPUT));
-    constexpr std::array<std::byte, 65> hex_literal_array{operator""_hex<util::detail::Hex(HEX_PARSE_INPUT)>()};
+    constexpr std::array<std::byte, 65> hex_literal_array{operator""_hex<util::hex_literals::detail::Hex(HEX_PARSE_INPUT)>()};
     auto hex_literal_span{MakeUCharSpan(hex_literal_array)};
     BOOST_CHECK_EQUAL_COLLECTIONS(hex_literal_span.begin(), hex_literal_span.end(), expected.begin(), expected.end());
 
-    const std::vector<std::byte> hex_literal_vector{operator""_hex_v<util::detail::Hex(HEX_PARSE_INPUT)>()};
+    const std::vector<std::byte> hex_literal_vector{operator""_hex_v<util::hex_literals::detail::Hex(HEX_PARSE_INPUT)>()};
     hex_literal_span = MakeUCharSpan(hex_literal_vector);
     BOOST_CHECK_EQUAL_COLLECTIONS(hex_literal_span.begin(), hex_literal_span.end(), expected.begin(), expected.end());
 
-    constexpr std::array<uint8_t, 65> hex_literal_array_uint8{operator""_hex_u8<util::detail::Hex(HEX_PARSE_INPUT)>()};
+    constexpr std::array<uint8_t, 65> hex_literal_array_uint8{operator""_hex_u8<util::hex_literals::detail::Hex(HEX_PARSE_INPUT)>()};
     BOOST_CHECK_EQUAL_COLLECTIONS(hex_literal_array_uint8.begin(), hex_literal_array_uint8.end(), expected.begin(), expected.end());
 
-    result = operator""_hex_v_u8<util::detail::Hex(HEX_PARSE_INPUT)>();
+    result = operator""_hex_v_u8<util::hex_literals::detail::Hex(HEX_PARSE_INPUT)>();
     BOOST_CHECK_EQUAL_COLLECTIONS(result.begin(), result.end(), expected.begin(), expected.end());
 
     result = ParseHex(HEX_PARSE_INPUT);
diff --git a/src/util/strencodings.h b/src/util/strencodings.h
index 1543de03ab..0f145d91c5 100644
--- a/src/util/strencodings.h
+++ b/src/util/strencodings.h
@@ -406,7 +406,7 @@ consteval uint8_t ConstevalHexDigit(const char c)
  *   ParseHex() does is because heap-based containers cannot cross the compile-
  *   time/runtime barrier.
  */
-inline namespace hex_literals {
+namespace hex_literals {
 namespace detail {
 
 template <size_t N>
@@ -427,16 +427,16 @@ struct Hex {
 
 } // namespace detail
 
-template <util::detail::Hex str>
+template <detail::Hex str>
 constexpr auto operator""_hex() { return str.bytes; }
 
-template <util::detail::Hex str>
+template <detail::Hex str>
 constexpr auto operator""_hex_u8() { return std::bit_cast<std::array<uint8_t, str.bytes.size()>>(str.bytes); }
 
-template <util::detail::Hex str>
+template <detail::Hex str>
 constexpr auto operator""_hex_v() { return std::vector<std::byte>{str.bytes.begin(), str.bytes.end()}; }
 
-template <util::detail::Hex str>
+template <detail::Hex str>
 inline auto operator""_hex_v_u8() { return std::vector<uint8_t>{UCharCast(str.bytes.data()), UCharCast(str.bytes.data() + str.bytes.size())}; }
 
 } // inline namespace hex_literals
diff --git a/src/util/string.h b/src/util/string.h
index 754972b4ed..418dd92ca3 100644
--- a/src/util/string.h
+++ b/src/util/string.h
@@ -18,50 +18,57 @@
 #include <vector>
 
 namespace util {
+namespace detail {
+constexpr unsigned int CountNumFormatSpecifiers(std::string_view str) 
+{
+    unsigned count_nor{0}; // Number of "normal" specifiers, like %s
+    unsigned count_pos{0}; // Max number in positional specifier, like %8$s
+    bool begin_f{false};
+    for (auto it{str.begin()}; it < str.end(); ++it) {
+        bool perc{*it == '%'};
+        if (begin_f) {
+            // Almost any char is allowed by tinyformat to be used in a
+            // format specifier, but a stricter check avoids bugs. For
+            // instance, a strict check prevents consuming the newline
+            // terminator in the format string "%\n". Also, excluding '0'
+            // avoids an out-of-range position error in tinyformat for the
+            // format string "%0$s\n".
+            if (std::string_view{"123456789aAcdfeEfFgGinopsuxX%"}.find(*it) == std::string_view::npos) throw "Invalid format specifier!";
+            auto it_num{it};
+            unsigned maybe_num{0};
+            while ('0' <= *it_num && *it_num <= '9') {
+                maybe_num *= 10;
+                maybe_num += *it_num - '0';
+                ++it_num;
+            };
+            if (it_num == it) {
+                // Non-positional specifier
+                count_nor += !perc;
+            } else {
+                // Positional specifier
+                if (*it_num != '$' || std::string_view{"aAcdfeEfFgGinopsuxX"}.find(*(++it_num)) == std::string_view::npos) {
+                    throw "Invalid positional format specifier!";
+                }
+                count_pos = std::max(count_pos, maybe_num);
+            }
+            begin_f = false;
+            continue;
+        }
+        if (perc) begin_f = true;
+    }
+    if (count_nor && count_pos) throw "Format specifiers must be all positional or all non-positional!";
+    unsigned count{count_nor | count_pos};
+    return count;
+}
+} // namespace detail
 /** Type to denote a format string that was checked at compile time */
 template <unsigned num_params>
 struct ConstevalFormatString {
     const char* const fmt;
-    consteval ConstevalFormatString(const char* str) : fmt{str} { Detail_CheckNumFormatSpecifiers(fmt); }
-    constexpr static void Detail_CheckNumFormatSpecifiers(std::string_view str)
+    consteval ConstevalFormatString(const char* str) : fmt{str} { CheckNumFormatSpecifiers(str); }
+    constexpr static void CheckNumFormatSpecifiers(std::string_view str)
     {
-        unsigned count_nor{0}; // Number of "normal" specifiers, like %s
-        unsigned count_pos{0}; // Max number in positional specifier, like %8$s
-        bool begin_f{false};
-        for (auto it{str.begin()}; it < str.end(); ++it) {
-            bool perc{*it == '%'};
-            if (begin_f) {
-                // Almost any char is allowed by tinyformat to be used in a
-                // format specifier, but a stricter check avoids bugs. For
-                // instance, a strict check prevents consuming the newline
-                // terminator in the format string "%\n". Also, excluding '0'
-                // avoids an out-of-range position error in tinyformat for the
-                // format string "%0$s\n".
-                if (std::string_view{"123456789aAcdfeEfFgGinopsuxX%"}.find(*it) == std::string_view::npos) throw "Invalid format specifier!";
-                auto it_num{it};
-                unsigned maybe_num{0};
-                while ('0' <= *it_num && *it_num <= '9') {
-                    maybe_num *= 10;
-                    maybe_num += *it_num - '0';
-                    ++it_num;
-                };
-                if (it_num == it) {
-                    // Non-positional specifier
-                    count_nor += !perc;
-                } else {
-                    // Positional specifier
-                    if (*it_num != '$' || std::string_view{"aAcdfeEfFgGinopsuxX"}.find(*(++it_num)) == std::string_view::npos) {
-                        throw "Invalid positional format specifier!";
-                    }
-                    count_pos = std::max(count_pos, maybe_num);
-                }
-                begin_f = false;
-                continue;
-            }
-            if (perc) begin_f = true;
-        }
-        if (count_nor && count_pos) throw "Format specifiers must be all positional or all non-positional!";
-        unsigned count{count_nor | count_pos};
+        unsigned count{detail::CountNumFormatSpecifiers(str)};
         if (num_params != count) throw "Format specifier count must match the argument count!";
     }
 };

```
</details>


Code after:
                continue;
            }

            unsigned maybe_num{0};
            while ('0' <= *it && *it <= '9') {
                maybe_num *= 10;
                maybe_num += *it - '0';
                ++it;
            };

            if (*it == '$') {
                // Positional specifier, like %8$s
                if (maybe_num == 0) throw "Positional format specifier must have position of at least 1";
                count_pos = std::max(count_pos, maybe_num);
                if (++it >= str.end()) throw "Format specifier incorrectly terminated by end of string";
            } else {
                // Non-positional specifier, like %s
                ++count_normal;
                ++it;
            }
            // The remainder "[flags][width][.precision][length]type" of the
            // specifier is not checked. Parsing continues with the next '%'.
        }
        if (count_normal && count_pos) throw "Format specifiers must be all positional or all non-positional!";
        unsigned count{count_normal | count_pos};
        if (num_params != count) throw "Format specifier count must match the argument count!";
    }
};

void ReplaceAll(std::string& in_out, const std::string& search, const std::string& substitute);

/** Split a string on any char found in separators, returning a vector.
 *
 * If sep does not occur in sp, a singleton with the entirety of sp is returned.
 *
 * Note that this function does not care about braces, so splitting
 * "foo(bar(1),2),3) on ',' will return {"foo(bar(1)", "2)", "3)"}.
 */
template <typename T = Span<const char>>
std::vector<T> Split(const Span<const char>& sp, std::string_view separators)
{
    std::vector<T> ret;
    auto it = sp.begin();
    auto start = it;
    while (it != sp.end()) {
        if (separators.find(*it) != std::string::npos) {
            ret.emplace_back(start, it);
            start = it + 1;
        }
        ++it;
    }