[libcxx-commits] [PATCH] D121478: [libc++] Add a lightweight overridable assertion handler

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In general I'm happy with this approach. I've some remarks. I like to see the CI green before making a final review.



================
Comment at: libcxx/docs/UsingLibcxx.rst:185
+functions in the library are ``noexcept``, and any exception thrown from the assertion handler
+will result in ``std::terminate`` being called.
+
----------------
Should we mention `std::unexpected`? This is used prior to C++17.


================
Comment at: libcxx/include/__assert:44
+_LIBCPP_OVERRIDABLE_FUNC_VIS
+void __libcpp_assertion_handler(char const* __file, int __line, char const* __expression, char const* __message);
 
----------------
I assume you didn't add `[[noreturn]]` to be compatible with C++98.


================
Comment at: libcxx/test/libcxx/assertions/assertion_handler.abort.pass.cpp:11
+
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_ASSERTIONS=1
+
----------------
Are there already tests `_LIBCPP_ASSERT` does nothing when `-D_LIBCPP_ENABLE_ASSERTIONS=0`?


================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:944
 
+#include <__assert>
 #include <__config>
----------------
This looks suspicious. Most headers don't include `__assert`.
Is this required for all headers? When it is I prefer to see required part be moved to `__config`.
Alternatively we need to enforce all headers include `version`. I added this, but there are no validations executed in the CI.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121478/new/

https://reviews.llvm.org/D121478