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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hodlinator, maflcko |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
273@@ -274,14 +274,12 @@ std::ostream& operator<<(std::ostream& os, const uint256& num);
274 class HasReason
275 {
276 public:
277- explicit HasReason(const std::string& reason) : m_reason(reason) {}
278- bool operator()(const std::exception& e) const
279- {
280- return std::string(e.what()).find(m_reason) != std::string::npos;
281- };
282+ explicit HasReason(const std::string_view& reason) : m_reason(reason) {}
278- bool operator()(const std::exception& e) const
279- {
280- return std::string(e.what()).find(m_reason) != std::string::npos;
281- };
282+ explicit HasReason(const std::string_view& reason) : m_reason(reason) {}
283+ bool operator()(std::string_view s) const { return s.find(m_reason) != std::string_view::npos; }
FailFmtWithError the full error was checked, now it checks that the error was found.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30314109955
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.
19@@ -19,11 +20,9 @@ inline void PassFmt(util::ConstevalFormatString<NumArgs> fmt)
20 decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt);
21 }
22 template <unsigned WrongNumArgs>
23-inline void FailFmtWithError(std::string_view wrong_fmt, std::string_view error)
24+inline void FailFmtWithError(std::string_view wrong_fmt, std::string error)
Unless HasReason gets a new ctor that takes non-ref string which we can std::move into (and it can move from internally into the member), we are now allocating extra string copies on every FailFmtWithError-call.
I prefer going back to string_view here.
Yet another possible alternative is to construct HasReason copies outside and pass them in. Nice to memory badbut slightly ugly using HasReason outside of the Boost macro.
0diff --git a/src/test/util_string_tests.cpp b/src/test/util_string_tests.cpp
1index 4f9aa197f9..5850102152 100644
2--- a/src/test/util_string_tests.cpp
3+++ b/src/test/util_string_tests.cpp
4@@ -20,9 +20,9 @@ inline void PassFmt(util::ConstevalFormatString<NumArgs> fmt)
5 decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt);
6 }
7 template <unsigned WrongNumArgs>
8-inline void FailFmtWithError(std::string_view wrong_fmt, std::string error)
9+inline void FailFmtWithError(std::string_view wrong_fmt, const HasReason& error)
10 {
11- BOOST_CHECK_EXCEPTION(util::ConstevalFormatString<WrongNumArgs>::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason(error));
12+ BOOST_CHECK_EXCEPTION(util::ConstevalFormatString<WrongNumArgs>::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, error);
13 }
14
15 BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
16@@ -61,23 +61,23 @@ BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
17 PassFmt<2>("%2$*3$d");
18 PassFmt<1>("%.*f");
19
20- auto err_mix{"Format specifiers must be all positional or all non-positional!"};
21+ HasReason err_mix{"Format specifiers must be all positional or all non-positional!"};
22 FailFmtWithError<1>("%s%1$s", err_mix);
23
24- auto err_num{"Format specifier count must match the argument count!"};
25+ HasReason err_num{"Format specifier count must match the argument count!"};
26 FailFmtWithError<1>("", err_num);
27 FailFmtWithError<0>("%s", err_num);
28 FailFmtWithError<2>("%s", err_num);
29 FailFmtWithError<0>("%1$s", err_num);
30 FailFmtWithError<2>("%1$s", err_num);
31
32- auto err_0_pos{"Positional format specifier must have position of at least 1"};
33+ HasReason err_0_pos{"Positional format specifier must have position of at least 1"};
34 FailFmtWithError<1>("%$s", err_0_pos);
35 FailFmtWithError<1>("%$", err_0_pos);
36 FailFmtWithError<0>("%0$", err_0_pos);
37 FailFmtWithError<0>("%0$s", err_0_pos);
38
39- auto err_term{"Format specifier incorrectly terminated by end of string"};
40+ HasReason err_term{"Format specifier incorrectly terminated by end of string"};
41 FailFmtWithError<1>("%", err_term);
42 FailFmtWithError<1>("%1$", err_term);
43 }
24+inline void FailFmtWithError(std::string_view wrong_fmt, std::string error)
25 {
26- using ErrType = const char*;
27- auto check_throw{[error](const ErrType& str) { return str == error; }};
28- BOOST_CHECK_EXCEPTION(util::ConstevalFormatString<WrongNumArgs>::Detail_CheckNumFormatSpecifiers(wrong_fmt), ErrType, check_throw);
29+ BOOST_CHECK_EXCEPTION(util::ConstevalFormatString<WrongNumArgs>::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason(error));
nit: Curly-maximalism.
0 BOOST_CHECK_EXCEPTION(util::ConstevalFormatString<WrongNumArgs>::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason{error});
19@@ -19,11 +20,9 @@ inline void PassFmt(util::ConstevalFormatString<NumArgs> fmt)
20 decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt);
21 }
22 template <unsigned WrongNumArgs>
23-inline void FailFmtWithError(std::string_view wrong_fmt, std::string_view error)
24+inline void FailFmtWithError(std::string_view wrong_fmt, HasReason error)
Avoid extra allocations (from my original diff):
0inline void FailFmtWithError(std::string_view wrong_fmt, const HasReason& error)
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:
0template <unsigned WrongNumArgs>
1inline void FailFmtWithError(std::string_view wrong_fmt, std::string_view error)
2{
3 auto check_throw{[error](std::string_view str) { return str == error; }};
4 BOOST_CHECK_EXCEPTION(util::ConstevalFormatString<WrongNumArgs>::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, check_throw);
5}
(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:
0template <unsigned WrongNumArgs>
1inline void FailFmtWithError(std::string_view wrong_fmt, std::string_view error)
2{
3 BOOST_CHECK_EXCEPTION(util::ConstevalFormatString<WrongNumArgs>::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason{error});
4}
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_viewconstructor forHasReasonbefore, 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
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
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)
Labels
Tests