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

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #26763 (ci: Treat IWYU violations as errors by hebasto)

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.

Concept ACK

Yeah, it might be fine, but I'd prefer to wait for C++23 consteval. Otherwise there seems to be the risk that an 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 Assert will 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.

Thanks, done in #23363 (comment)

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.

I tried this and it seems to compile, but I don't think it is worth it:

diff --git a/src/util/check.h b/src/util/check.h
index a02a1de8dc..f15117daef 100644
--- a/src/util/check.h
+++ b/src/util/check.h
@@ -21,9 +21,11 @@ public:
     NonFatalCheckError(std::string_view msg, std::string_view file, int line, std::string_view func);
 };
 
+consteval std::string_view inline_String_View(const char* sv) { return std::string_view{sv}; }
+
 /** Helper for CHECK_NONFATAL() */
 template <typename T>
-T&& inline_check_non_fatal(LIFETIMEBOUND T&& val, const char* file, int line, const char* func, const char* assertion)
+T&& inline_check_non_fatal(LIFETIMEBOUND T&& val, std::string_view file, int line, std::string_view func, std::string_view assertion)
 {
     if (!val) {
         throw NonFatalCheckError{assertion, file, line, func};
@@ -40,7 +42,7 @@ void assertion_fail(std::string_view file, int line, std::string_view func, std:
 
 /** Helper for Assert()/Assume() */
 template <bool IS_ASSERT, typename T>
-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)
+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)
 {
     if constexpr (IS_ASSERT
 #ifdef ABORT_ON_FAILED_ASSUME
@@ -74,7 +76,7 @@ T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] const char* f
     inline_check_non_fatal(condition, __FILE__, __LINE__, __func__, #condition)
 
 /** Identity function. Abort if the value compares equal to zero */
-#define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
+#define Assert(val) inline_assertion_check<true>(val, inline_String_View(__FILE__), __LINE__, __func__, #val)
 
 /**
  * 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)