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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
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.
+
----------------
Mordante wrote:
> Should we mention `std::unexpected`? This is used prior to C++17.
Since I'm specifically talking about `noexcept` and not `throw()`, I think this is sufficient -- `std::unexpected` will never be called if an exception is thrown from a `noexcept` function. Please let me know if you disagree and would still like me to add precision, and I can do it.


================
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);
 
----------------
Mordante wrote:
> I assume you didn't add `[[noreturn]]` to be compatible with C++98.
Actually, I omitted `[[noreturn]]` because I wanted to leave the door open for an assertion handler to return. Yes, I know the documentation says users shouldn't do it, but I was thinking that perhaps there are useful applications I'm not thinking about.

I'm not married to this -- do you think I should strengthen the signature and force everybody's handler to be `__attribute__((noreturn))`? I assume that works with all supported compilers, I can confirm if we agree we want to do this.


================
Comment at: libcxx/test/libcxx/assertions/assertion_handler.abort.pass.cpp:11
+
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_ASSERTIONS=1
+
----------------
Mordante wrote:
> Are there already tests `_LIBCPP_ASSERT` does nothing when `-D_LIBCPP_ENABLE_ASSERTIONS=0`?
I don't think so. I'll add one, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121478