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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__assert:44
+_LIBCPP_OVERRIDABLE_FUNC_VIS
+void __libcpp_assertion_handler(char const* __file, int __line, char const* __expression, char const* __message);
 
----------------
ldionne wrote:
> Mordante wrote:
> > I assume you didn't add `[[noreturn]]` to be compatible with C++98.
> Actually, I omitted `[[noreturn]]` because I wanted to leave the door open for an assertion handler to return. Yes, I know the documentation says users shouldn't do it, but I was thinking that perhaps there are useful applications I'm not thinking about.
> 
> I'm not married to this -- do you think I should strengthen the signature and force everybody's handler to be `__attribute__((noreturn))`? I assume that works with all supported compilers, I can confirm if we agree we want to do this.
I don't think the strengthening is required. It won't work in C++03 and when there are indeed user who want to return it really becomes UB. I can think of one use case: users using a sanitizer and only want to log asserts to see whether the sanitzers pick up an error.


================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:944
 
+#include <__assert>
 #include <__config>
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121478