refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions

l0rinc commented at 0:54 am on October 8, 2025: contributor

Works around Clang code generation bug when targeting Windows where where catching const char* exceptions by reference is broken, see https://github.com/mstorsjo/llvm-mingw/issues/522

Following C++ Core Guidelines E.15 “Throw by value, catch exceptions from a hierarchy by reference”, we throw std::string_view instead of string literals. Note that clang-tidy mentions string literals not being flagged, but it’s still an llvm-mingw issue so we need to adjust anyway. Array-to-pointer conversion makes string literal exceptions particularly fragile across platforms.

Also deleted HasReason::operator()(const char*) to prevent future reliance on string literal exception handling.

The error was reproduced by adding a temporary nightly build pointing to https://github.com/l0rinc/bitcoin/pull/44. On master the failure can be seen in https://github.com/l0rinc/bitcoin-core-nightly/pull/2

DrahtBot added the label Refactoring on Oct 8, 2025

DrahtBot commented at 0:54 am on October 8, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33569.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hodlinator
Concept ACK	Dorex45
Stale ACK	purpleKarrot

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

Hex string must fit exactly -> Hex string must have exactly the required length [“fit exactly” is incomplete/ambiguous; this clarifies the intended meaning]

drahtbot_id_5_m

in src/util/strencodings.h:334 in 362d43a659

330@@ -331,7 +331,7 @@ consteval uint8_t ConstevalHexDigit(const char c)
331     if (c >= '0' && c <= '9') return c - '0';
332     if (c >= 'a' && c <= 'f') return c - 'a' + 0xa;
333 
334-    throw "Only lowercase hex digits are allowed, for consistency";
335+    throw std::runtime_error("Only lowercase hex digits are allowed, for consistency");

hodlinator commented at 7:33 am on October 8, 2025:

nit: Using runtime_error everywhere in these heavily consteval/constexpr contexts feels slightly off, so I agree with what you suggested in the PR description.

0    throw std::invalid_argument{"Only lowercase hex digits are allowed, for consistency"};

l0rinc commented at 5:26 pm on October 8, 2025:

Went with std::string_view instead, thanks.

purpleKarrot commented at 7:48 am on October 8, 2025: contributor

ACK 362d43a659224f9969b268dcc859162006460214

Note that some of the std::runtime_error instances can likely be std::invalid_argument or std::logic_error - if the reviewers have stronger preferences, I can adjust.

Long term, it should be evaluated whether some contracts could be tightened by turning runtime error cases into preconditions.

in src/test/util/setup_common.h:310 in 362d43a659 outdated

304@@ -305,7 +305,8 @@ class HasReason
305 public:
306     explicit HasReason(std::string_view reason) : m_reason(reason) {}
307     bool operator()(std::string_view s) const { return s.find(m_reason) != std::string_view::npos; }
308-    bool operator()(const std::exception& e) const { return (*this)(e.what()); }
309+    bool operator()(const std::exception& e) const { return (*this)(std::string_view{e.what()}); }
310+    bool operator()(const char* s) const = delete;
311

hodlinator commented at 8:09 am on October 8, 2025:

I don’t get it, https://github.com/hebasto/bitcoin-core-nightly/actions/runs/18313967330/job/52151177201#step:6:401 logs: test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}"

Looking at where the error message comes from - https://www.boost.org/doc/libs/1_87_0/boost/test/tools/old/interface.hpp, it has:

 0#define BOOST_CHECK_THROW_IMPL( S, E, P, postfix, TL )                                  \
 1do {                                                                                    \
 2    try {                                                                               \
 3        BOOST_TEST_PASSPOINT();                                                         \
 4        S;                                                                              \
 5        BOOST_TEST_TOOL_IMPL( 2, false, "exception " BOOST_STRINGIZE(E) " expected but not raised", \
 6                              TL, CHECK_MSG, _ );                                       \
 7    } catch( E const& ex ) {                                                            \
 8        boost::ignore_unused( ex );                                                     \
 9        BOOST_TEST_TOOL_IMPL( 2, P,                                                     \
10                              "exception \"" BOOST_STRINGIZE( E )"\" raised as expected" postfix,   \
11                              TL, CHECK_MSG, _  );                                      \
12    }                                                                                   \
13} while( 0 )                                                                            \
14...
15#define BOOST_CHECK_EXCEPTION( S, E, P )    BOOST_CHECK_THROW_IMPL( S, E, P( ex ), \
16              ": validation on the raised exception through predicate \"" BOOST_STRINGIZE(P) "\"", CHECK )

…this indicates that the exception type matched in the catch in BOOST_CHECK_THROW_IMPL, and HasReason was successfully compiled to take the exception type. But for some reason HasReason::operator(std::string_view) fails.

Staring at the macros, maybe it’s that HasReason{error}(ex) somehow calls HasReason::operator()(const std::exception& e) if the std::exception constructor is not explicit in the std lib used.. but it should still find the string.

What is going on here?

hebasto commented at 8:26 am on October 8, 2025:

See https://github.com/mstorsjo/llvm-mingw/issues/522.

hodlinator commented at 9:08 am on October 8, 2025:

Great! Would prefer something like this diff applied to the PR description and commit message to make it even clearer that this is primarily a workaround for a compiler issue and secondarily abiding by guidelines:

0 Fixes MinGW/libc++ test failures where `BOOST_CHECK_EXCEPTION` with `const char*` exceptions
1- failed due to platform-specific exception handling differences.
2+ is broken, see https://github.com/mstorsjo/llvm-mingw/issues/522

Edit:

0 Fixes MinGW/libc++ test failures where `BOOST_CHECK_EXCEPTION` 
1- with `const char*` exceptions failed due to platform-specific exception handling differences.
2+ catching `const char*`-exceptions by reference is broken, see https://github.com/mstorsjo/llvm-mingw/issues/522

l0rinc commented at 5:26 pm on October 8, 2025:

Updated the description, thanks!

hodlinator commented at 7:59 am on October 9, 2025:

Thanks!

Nice mention regarding clang-tidy’s misc-throw-by-value-catch-by-reference allowing for throwing & catching string literals.

++nit

Now that there is a proposed fix in https://github.com/llvm/llvm-project/pull/162546, we can see that although the parent issue to this one (#33550) is about libc++, that is not the case here. Going by the llvm-project PR, the issue is in the Clang compiler’s code generation (when compiling for Windows which uses SEH).

Commit message could be made more precise by

0- Works around MinGW/libc++ bug where
1+ Works around Clang code generation bug when targeting Windows where

Or you could at least adjust the PR description if invalidating ACKs is a concern.

l0rinc commented at 12:38 pm on October 9, 2025:

Updated the PR description, thanks.

hodlinator commented at 8:17 am on October 8, 2025: contributor

Reviewed 362d43a659224f9969b268dcc859162006460214

(I agree with the general rule of not throwing random pointer types, but when its const char* from the static text section of the executable that are never deallocated during the process lifetime I think it’s acceptable).

maflcko commented at 9:05 am on October 8, 2025: member

lgtm, but it seems this is a workaround for an upstream bug?

In any case, the reason why a raw string literal was thrown, because the primary goal of this is inside a consteval context, and it avoids an include. I guess you’d have to add the include here, if you start using runtime_error. Though, that feels off for this context, as mentioned previously.

Out of curiosity, I wonder, would it work if a string_view was thrown?

fanquake commented at 12:37 pm on October 8, 2025: member

but it seems this is a workaround for an upstream bug?

Yep, looks like upstream has confirmed it’s an upsteam bug: https://github.com/mstorsjo/llvm-mingw/issues/522#issuecomment-3381159016.

mstorsjo commented at 2:33 pm on October 8, 2025: none

Also, FWIW, the issue isn’t about throwing a const char *, it’s about whether the pointer is caught by value or by reference, as catch (const char*) or catch (const char* &). For larger types (like classes), I can see some performance benefit from catching by reference, but for a pointer sized value like this, there shouldn’t be any performance difference. It’s of course possible that the distinction isn’t quite as easy and clear between those two as it was in the reduced testcase in https://github.com/mstorsjo/llvm-mingw/issues/522 though.

l0rinc renamed this:
~~refactor: replace `const char*` exceptions with `std::runtime_error`~~
refactor: throw exception objects instead of `const char*` pointers
on Oct 8, 2025

refactor: Throw `std::string_view` instead of `const char*` in constexpr/consteval functions

Works around MinGW/libc++ bug where `BOOST_CHECK_EXCEPTION` catching `const char*` exceptions by reference is broken, see https://github.com/mstorsjo/llvm-mingw/issues/522

While throwing string literals is generally acceptable (clang-tidy explicitly allows it as idiomatic), using proper exception objects provides better type safety and avoids this platform-specific issue.

Also deleted `HasReason::operator()(const char*)` to prevent future reliance on const char* exception handling.

Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>

ec2476c811

l0rinc force-pushed on Oct 8, 2025

l0rinc renamed this:
~~refactor: throw exception objects instead of `const char*` pointers~~
refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions
on Oct 8, 2025

l0rinc commented at 5:27 pm on October 8, 2025: contributor

Thank you for the quick reviews!

would it work if a string_view was thrown?

While that doesn’t really help with providing more context for the failure type, I can sympathize with the dependency lowering goal for consteval/constexp functions. https://github.com/l0rinc/bitcoin-core-nightly/pull/3 indicates this also fixes the issue, so I have used that, updated commit and PR description to reflect it.

whether some contracts could be tightened by turning runtime error cases into preconditions

That would indeed be a reasonable followup, thanks for the hint!

in src/uint256.h:127 in ec2476c811

123@@ -124,7 +124,7 @@ class base_blob
124 template <unsigned int BITS>
125 consteval base_blob<BITS>::base_blob(std::string_view hex_str)
126 {
127-    if (hex_str.length() != m_data.size() * 2) throw "Hex string must fit exactly";

hebasto commented at 5:35 pm on October 8, 2025:

If we go down this path, could the linter be assigned to this task as well?

UPD: … or clang-tidy module?

l0rinc commented at 5:51 pm on October 8, 2025:

I also thought of this, it’s why I’ve mentioned https://clang.llvm.org/extra/clang-tidy/checks/misc/throw-by-value-catch-by-reference.html in the descripton in the latest push - but it seems string literal throws are tolerated there. I don’t mind adding such a check, but it seemed to me contrib/devtools/bitcoin-tidy isn’t actively developed.

in src/util/strencodings.h:334 in ec2476c811

330@@ -331,7 +331,7 @@ consteval uint8_t ConstevalHexDigit(const char c)
331     if (c >= '0' && c <= '9') return c - '0';
332     if (c >= 'a' && c <= 'f') return c - 'a' + 0xa;
333 
334-    throw "Only lowercase hex digits are allowed, for consistency";
335+    throw std::string_view("Only lowercase hex digits are allowed, for consistency");

hodlinator commented at 8:08 am on October 9, 2025:

meganit: Uses () in 3 throws and {} in 3.

hodlinator approved

hodlinator commented at 8:10 am on October 9, 2025: contributor

crACK ec2476c8115e249ddfa3c8a1e8c892d5f822167b

in src/test/util_string_tests.cpp:40 in ec2476c811

36@@ -37,7 +37,7 @@ void PassFmt(ConstevalFormatString<NumArgs> fmt)
37 template <unsigned WrongNumArgs>
38 void FailFmtWithError(const char* wrong_fmt, std::string_view error)
39 {
40-    BOOST_CHECK_EXCEPTION(CheckNumFormatSpecifiers<WrongNumArgs>(wrong_fmt), const char*, HasReason{error});
41+    BOOST_CHECK_EXCEPTION(CheckNumFormatSpecifiers<WrongNumArgs>(wrong_fmt), std::string_view, HasReason{error});

maflcko commented at 9:55 am on October 9, 2025:

nit: I guess a more minimal one-line temporary(?) workaround would be to just add & here? Ref #33569 (comment)

0    BOOST_CHECK_EXCEPTION(CheckNumFormatSpecifiers<WrongNumArgs>(wrong_fmt), const char*&, HasReason{error});

I don’t mind adding such a check, but it seemed to me contrib/devtools/bitcoin-tidy isn’t actively developed.

In theory we could add a tidy check for this, but I wonder how much it is worth it for a temporary(?) workaround.

l0rinc commented at 12:40 pm on October 9, 2025:

a more minimal one-line temporary(?) workaround

I don’t like throwing primitives anyway, so I don’t think the current change is a workaround only.

maflcko commented at 1:08 pm on October 9, 2025:

https://clang.llvm.org/extra/clang-tidy/checks/misc/throw-by-value-catch-by-reference.html says “the usage of string literals is idiomatic.” But no strong opinion.

Dorex45 commented at 6:32 am on October 10, 2025: none

Related to #33550 (comment)

Works around Clang code generation bug when targeting Windows where where catching const char* exceptions by reference is broken, see mstorsjo/llvm-mingw#522

Following C++ Core Guidelines E.15 “Throw by value, catch exceptions from a hierarchy by reference”, we throw std::string_view instead of string literals. Note that clang-tidy mentions string literals not being flagged, but it’s still an llvm-mingw issue so we need to adjust anyway. Array-to-pointer conversion makes string literal exceptions particularly fragile across platforms.

Also deleted HasReason::operator()(const char*) to prevent future reliance on string literal exception handling.

The error was reproduced by adding a temporary nightly build pointing to l0rinc#44. On master the failure can be seen in l0rinc/bitcoin-core-nightly#2

ACK

refactor: throw std::string_view instead of const char* in constexpr/consteval functions #33569

Code Coverage & Benchmarks

Reviews

LLM Linter (✨ experimental)

++nit

refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions #33569