[libcxx-commits] [PATCH] D121478: [libc++] Add a lightweight overridable assertion handler

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision as: Mordante.
Mordante added a comment.

LGTM when the CI passes.



================
Comment at: libcxx/include/__availability:177
+    // should be avoided.
+#   define _LIBCPP_AVAILABILITY_DEFAULT_ASSERTION_HANDLER
+
----------------
Nice solution using the availability macro!


================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:944
 
+#include <__assert>
 #include <__config>
----------------
ldionne wrote:
> Mordante wrote:
> > ldionne wrote:
> > > 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.
> > The only issue is, that not every header is required to include `version`. 
> > `git grep -L --no-recursive 'include.*version' -- libcxx/include/` shows a lot of our c headers, `barrier` and `initializer_list`.
> > So it we use version we need to add that to these headers.
> > Since all new C++ proposals add a new feature-test-macro, I think there's not a big chance of regressions.
> > 
> > So I like the idea users only need to include any Standard header, but then we need to make sure it works for all of them.
> > I think any header is safer, when it requires `<cassert>` I'm not sure whether a tool like IWYU will remove the include by accident.
> > 
> > I think using this handler for `assert` sounds interesting. But I see a bit of an issue, we only ship `cassert` and not `assert.h`. I'm not sure whether that can cause subtile issues depending on whether `cassert` or `assert.h` is included. But when we go that route it should be done in a separate patch.
> I agree that any header should be sufficient. I propose this because the patch is starting to grow quite a bit: For now, I'll require that `<__assert>` be included before defining a custom handler. Then, I will relax this requirement in future patches:
> 
> 1. Refactor the generated tests that check for per-header properties -- each header should be tested in a different translation unit instead.
> 2. Add a test that checks that all public headers provide a declaration for the assertion handler
> 
> Once we have that framework in place, we can also easily add a test to check that e.g. all public headers define e.g. `_LIBCPP_VERSION` (we have such tests today but they are written manually, see e.g. `libcxx/test/libcxx/numerics/cfenv/version.pass.cpp`).
> 
SGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121478