test: generalize HasReason and use it in FailFmtWithError #30921

Standardized boost exception checking in recent tests introduced in #30546 (review) by extending HasReason to accept const char* through string_view in operator().

Note that HasReason only checks partial matches - but since we're specifying the whole error string, it doesn't affect us in this case.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage

For detailed information about the code coverage, see the test coverage report.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #30933 (test: Prove+document ConstevalFormatString/tinyformat parity by hodlinator)
• #30929 (log: Enforce trailing newline at compile time by maflcko)

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.

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Debug: https://github.com/bitcoin/bitcoin/runs/30314109955}

<details><summary>Hints</summary>

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

Concept ACK

As previously stated I would most prefer the parameters to FailFmtWithError were reverted so it took a string_view. The current version (regrettably suggested as an alternative by yours truly) uses HasReason "out of context" just to be more friendly to memory when using HasReason repetitively. I wouldn't blame others for finding it a step back from a readability standpoint.

The whole reason of the PR was to use HasReason, but doing so and using it in a new way outside the Boost macros feels a bit forced. Apologies for suggesting it. Maybe introducing a new type, local to the test would be more natural. In fact I like the original version better, minus ErrType. Making it:

template <unsigned WrongNumArgs>
inline void FailFmtWithError(std::string_view wrong_fmt, std::string_view error)
{
    auto check_throw{[error](std::string_view str) { return str == error; }};
    BOOST_CHECK_EXCEPTION(util::ConstevalFormatString<WrongNumArgs>::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, check_throw);
}

(It's more optimal than the current version from a memory standpoint and avoids touching setup_common.h).

But in following with the spirit of this PR, I think the following would be a good solution, using HasReason as its name intends it to be used:

template <unsigned WrongNumArgs>
inline void FailFmtWithError(std::string_view wrong_fmt, std::string_view error)
{
    BOOST_CHECK_EXCEPTION(util::ConstevalFormatString<WrongNumArgs>::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason{error});
}

It requires changing the HasReason constructor to take a string_view instead of const string& which is arguably a step in the right direction anyway (+ relies on your other changes to that type).

Thanks for your detailed explanation @hodlinator.

optimal than the current version

We're talking about something extremely trivial, inside tests, it could be a million times more expensive and still wouldn't matter...

It requires changing the HasReason constructor to take a string_view instead of const string& which is arguably a step in the right direction anyway (+ relies on your other changes to that type).

I had a string_view constructor for HasReason before, but it was regarded as premature optimization, so I've reverted that part.

but doing so and using it in a new way outside the Boost macros feels a bit forced

I kinda' like your HasReason err {"..."} suggestions actually - It's basically a simple validation lambda, same as before, but now it has a name (which may be a bit weird, but the concept is quote elegant).

I had a string_view constructor for HasReason before, but it was regarded as premature optimization, so I've reverted that part.

Is there a way to see complete force-push history? Since you force-pushed twice in a short timespan I'm unable to see your first version of the PR. I suspect from maflcko's comment that you may have been changing HasReason::m_reason itself into a string_view as well(?), which I agree would increase the risk of dangling issues.

Is there a way to see complete force-push history?

GitHub sometimes(?) sends commit hashes by email, it was 8ffb79805d3589a86ca97e976b9d271cd8062ae8

changing HasReason::m_reason itself into a string_view as well(?), which I agree would increase the risk of dangling issues

Since string literals have static storage duration, referencing them via std::string_view should be safe (i.e. for HasReason storage as const std::string_view m_reason). But you're both right that lifetime management can be more complex than what we have in this test, so using an std::string copy avoids dangling references. Thanks for the reviews, pushed.

ACK 6c3c619b35cc03e883f9d2b3326f906aedde9ba7

Reuses the existing HasReason-idiom in readable way while also being somewhat more efficient memory-wise thanks to the HasReason-ctor change.

nit is back: #30921 (review)

review ACK 6c3c619b35cc03e883f9d2b3326f906aedde9ba7