[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 10 inline comments as done.
ldionne added inline comments.


================
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>
----------------
Mordante wrote:
> Do users need to do this in every translation unit? (Looking at the macro I assume yes.)
Yes, they do, but as we've mentioned, this should be done from the compiler command-line.


================
Comment at: libcxx/docs/UsingLibcxx.rst:167
+  #define _LIBCPP_ASSERTION_HANDLER(info) ::HelloWorldAssertionHandler(info)
+  #include <__assert>
+
----------------
Quuxplusone wrote:
> Mordante wrote:
> > 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.
> +1 Mordante's idea of `<__libcpp_assertion_handler>`, except that I worry that "one header per entry point" is too fine a level of granularity. Like maybe it should be `<__libcpp_assert>` and pull in all the stuff relevant to assertion handlers (`__libcpp_assertion_handler`, `__libcpp_assertion_handler_type`, `__libcpp_set_assertion_handler`...). And then that's just `<__assert>` (i.e. what Louis has done) but with a `libcpp` on the front. Which might still be a nice touch.
> 
> If we were to rename `__assert` to `__libcpp_assert`, would that hurt any users? (No, because `__assert` hasn't shipped yet?)
> If we were to rename it, what would be connoted by the symmetry between `__libcpp_assert` and `__libcpp_version`? Would that be a good thing or a bad thing? (Is `__libcpp_version` even shipped? I have never bothered to understand its job.)
> 
> Is this a job for a subdirectory, like `<libcpp/assert.h>` or `<ext/libcpp_assertions.h>` or something?
I'm not sure I understand the request here. We already have a header that includes "everything related to assertions", and the name of that header is `<__assert>`. We can discuss changing the name of that header, no problem, but I'm not sure I understand your request if it's not just to rename the header. What would `< __libcpp_assertion_hander>` contain?


================
Comment at: libcxx/include/__availability:87
+    // assertions in the library, or must provide their own assertion handler.
+#   define _LIBCPP_AVAILABILITY_ASSERTION_HANDLER
+
----------------
Mordante wrote:
> What happens when a user enables this on Apple? Will it result in a compilation or a linkage error?
If a user tries to enable assertions on Apple and is deploying to a platform that doesn't support the default handler (which is all platforms for the time being), they will get a compilation error saying they are using the default handler, which hasn't been introduced yet.

If they define their own custom handler, they won't get any error.

Eventually, once we actually ship a dylib with support for the assertion handler, this will move from `__attribute__((unavailable))` on line 160 below to `__attribute__((availability(macosx,strict,introduced=SOME VERSION)))`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121123