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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 2 inline comments as done.
ldionne 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
----------------
philnik wrote:
> 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?
I understand this is a bit surprising, however from a vendor's perspective, it may be relevant to ship a "hardened" library where assertions are enabled by default *for everyone*, by default. I know there are platforms like that out there, and in fact it is one of the use cases motivating this work.


================
Comment at: libcxx/docs/UsingLibcxx.rst:167
+  #define _LIBCPP_ASSERTION_HANDLER(info) ::HelloWorldAssertionHandler(info)
+  #include <__assert>
+
----------------
philnik wrote:
> 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.
I agree it makes some sense, however we still have the problem that we need to include `<iosfwd>` from this header.

In other words, I don't think we can easily change this part of the user-facing API unless we agree to drop `<iosfwd>` from `<__assert>` (and in that case we can even include it from e.g. `<__config>`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121123