Seems confusing and fragile to require calling code to call c_str() when passing a read-only view of a std::string.
Fix that by using std::string_view, which can be constructed from string literals and std::string.
Also, remove the now unused c_str() from src/wallet/bdb.cpp.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v, aureleoules, theStack |
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.
ACK fab958290b84037fad1d78ffb2bce34d2b0e8c36
Pretty straightforward change that makes the code more robust. I’m curious why you reverted from fab5500 to fab9582, I think inline_check_non_fatal and inline_assertion_check could benefit from string_view too? Compilation and unit tests seem fine here - so I’m probably missing some nuance?
Assert will eat linear time at runtime in the length of the message, even if the assert is not hit.
Yeah, it might be fine, but I’d prefer to wait for C++23 consteval. Otherwise there seems to be the risk that an
Assertwill eat linear time at runtime in the length of the message, even if the assert is not hit.
If I understand correctly: because if we start using these helpers to take string_view, they’d have to calculate the length of the array at run-time every time Assert/Assume functions are called, and since those are called quite a lot and most of the time we expect them not to fail (i.e. the string_views go completely unused), that would be a waste. With consteval (I think it’s since C++20 already?) that run-time complexity would be avoided since everything is inlined and calculated at compile-time.
If so, nit: worth adding a little // TODO comment to document that with C++20 we can use consteval and string_view? Personally, I find those quite helpful.
If so, nit: worth adding a little
// TODOcomment to document that with C++20 we can useconstevalandstring_view? Personally, I find those quite helpful.
I tried this and it seems to compile, but I don’t think it is worth it:
0diff --git a/src/util/check.h b/src/util/check.h
1index a02a1de8dc..f15117daef 100644
2--- a/src/util/check.h
3+++ b/src/util/check.h
4@@ -21,9 +21,11 @@ public:
5 NonFatalCheckError(std::string_view msg, std::string_view file, int line, std::string_view func);
6 };
7
8+consteval std::string_view inline_String_View(const char* sv) { return std::string_view{sv}; }
9+
10 /** Helper for CHECK_NONFATAL() */
11 template <typename T>
12-T&& inline_check_non_fatal(LIFETIMEBOUND T&& val, const char* file, int line, const char* func, const char* assertion)
13+T&& inline_check_non_fatal(LIFETIMEBOUND T&& val, std::string_view file, int line, std::string_view func, std::string_view assertion)
14 {
15 if (!val) {
16 throw NonFatalCheckError{assertion, file, line, func};
17@@ -40,7 +42,7 @@ void assertion_fail(std::string_view file, int line, std::string_view func, std:
18
19 /** Helper for Assert()/Assume() */
20 template <bool IS_ASSERT, typename T>
21-T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] const char* file, [[maybe_unused]] int line, [[maybe_unused]] const char* func, [[maybe_unused]] const char* assertion)
22+T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] std::string_view file, [[maybe_unused]] int line, [[maybe_unused]] std::string_view func, [[maybe_unused]] std::string_view assertion)
23 {
24 if constexpr (IS_ASSERT
25 #ifdef ABORT_ON_FAILED_ASSUME
26@@ -74,7 +76,7 @@ T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] const char* f
27 inline_check_non_fatal(condition, __FILE__, __LINE__, __func__, #condition)
28
29 /** Identity function. Abort if the value compares equal to zero */
30-#define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
31+#define Assert(val) inline_assertion_check<true>(val, inline_String_View(__FILE__), __LINE__, __func__, #val)
32
33 /**
34 * Assume is the identity function.
Seems better to just remove the helper functions and macros and use a plain C++20 function, which would also make the compiler warning more useful, see #28999 (comment)