[libcxx-commits] [PATCH] D121123: [libc++] Add a way to enable lightweight assertions independent of the debug mode

Mon Mar 7 08:23:37 PST 2022

philnik added inline comments.

================
Comment at: libcxx/CMakeLists.txt:85
 # Basic options ---------------------------------------------------------------
-option(LIBCXX_ENABLE_ASSERTIONS "Enable assertions independent of build mode." OFF)
+option(LIBCXX_ENABLE_ASSERTIONS
+  "Enable assertions inside the compiled library, and at the same time make it the
----------------
ldionne wrote:
> A point of discussion I expect is whether `LIBCXX_ENABLE_ASSERTIONS` at CMake configure-time should control *both* whether the dylib contains assertions and what the default is for user code. I've done it that way for now because I didn't really expect the need for additional customizeability, but I could be convinced otherwise.
> 
> TLDR, the one thing you can't do with the current design is build libc++.dylib **with** assertions enabled, but have assertions disabled inside user code by default (or the other way around, i.e. no assertions in the dylib, but user code builds with assertions by default).
I would expect it to be off by default regardless of how the vendor configures it. What's the point of a default if it can be different on every platform anyways?

================
Comment at: libcxx/docs/UsingLibcxx.rst:167
+  #define _LIBCPP_ASSERTION_HANDLER(info) ::HelloWorldAssertionHandler(info)
+  #include <__assert>
+
----------------
ldionne wrote:
> I am not very happy about requiring users to include an internal header to customize the assertion handler. IMO that's kind of hacky. I thought about using `<version>` instead, but that would require `<version>` to include `<__assert>`, and unfortunately that's a no-go because `<__assert>` needs to include `<iosfwd>` right now (due to the legacy debug mode support).
Why not put it in `<cassert>`? That would make the most sense IMO.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121123