[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/utils/generate_feature_test_macro_components.py:944
 
+#include <__assert>
 #include <__config>
----------------
Mordante wrote:
> This looks suspicious. Most headers don't include `__assert`.
> Is this required for all headers? When it is I prefer to see required part be moved to `__config`.
> Alternatively we need to enforce all headers include `version`. I added this, but there are no validations executed in the CI.
> 
We need to settle on a way users can be *guaranteed* that `std::__libcpp_assertion_handler` will be declared. I'm currently using "include any libc++ header and it will be declared", but it could be something else, like "include `<cassert>` and you can be guaranteed it'll be declared", or whatever. It could also be something more libc++ specific like "include `<__libcpp_assertion_handler>` and it will be declared", but I like that less.

WDYT about including `<__assert>` in `<cassert>`, and saying that users only need to include `<cassert>` in order to define a custom assertion handler? The only thing I'd be slightly worried about is giving the false impression that this assertion handler will be used for `assert(...)`, which is not the case. Actually, while we're at it, do we think that the `assert` macro should use the custom assertion handler? That's a really good question, I guess.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121478