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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I haven't looked at the code and design in depth, but have some remarks already.



================
Comment at: libcxx/docs/UsingLibcxx.rst:166
+  // In HelloWorldConfig.h (this must always be included before any other libc++ header)
+  #define _LIBCPP_ASSERTION_HANDLER(info) ::HelloWorldAssertionHandler(info)
+  #include <__assert>
----------------
Do users need to do this in every translation unit? (Looking at the macro I assume yes.)


================
Comment at: libcxx/docs/UsingLibcxx.rst:167
+  #define _LIBCPP_ASSERTION_HANDLER(info) ::HelloWorldAssertionHandler(info)
+  #include <__assert>
+
----------------
ldionne wrote:
> Quuxplusone wrote:
> > ldionne wrote:
> > > 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>`).
> > Tangent FWIW: it seems like we could use a `<__stringfwd>` helper header.
> I agree, and it's useful for e.g. D116950 anyways.
I think when we need an internal header it would be a good idea to add a new header specifically for that purpose.
`__libcpp_assertion_hander` for example. That header can then include `__assert`, but that way we can change our implementation without breaking user code.


================
Comment at: libcxx/docs/UsingLibcxx.rst:171
+  // for simplicity in this example.
+  inline void HelloWorldAssertionHandler(std::__libcpp_assertion_info info) {
+      std::printf("Assertion %s failed at %s:%d, more info: %s",
----------------
How about adding a `[[noreturn]]` attribute on the function?


================
Comment at: libcxx/include/__assert:21
 
+// This is for backwards compatibility with folks that might have been enabling
+// assertions through the Debug mode previously.
----------------
I think this looks better in code that is visible to users.


================
Comment at: libcxx/include/__availability:87
+    // assertions in the library, or must provide their own assertion handler.
+#   define _LIBCPP_AVAILABILITY_ASSERTION_HANDLER
+
----------------
What happens when a user enables this on Apple? Will it result in a compilation or a linkage error?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121123