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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/docs/UsingLibcxx.rst:158-161
+Replacing the default assertion handler is done by defining the ``_LIBCPP_ASSERTION_HANDLER`` macro,
+which is called with an instance of ``std::__libcpp_assertion_info``. Generally speaking,
+``_LIBCPP_ASSERTION_HANDLER(assertion_info)`` should expand to a fully qualified function call.
+For example:
----------------
ldionne wrote:
> Quuxplusone wrote:
> > ```
> > // In HelloWorldConfig.h (this must always be included before any other libc++ header)
> > ```
> > I think this has come up before, like in the past six months: We shouldn't encourage users to `#define _LIBCPP_WHATEVER` at the top of their file, because that will tend to get moved down, or not work when their file is a .h, or break when they move to modules, or whatever. We should encourage them to pass `-D_LIBCPP_WHATEVER` in their build system's C(XX)FLAGS instead.
> > 
> > (Yikes, you're even suggesting that they //do// put it in a .h, with a note that this header //must// be included first in any .cpp file that needs it. That's asking for trouble.)
> I agree, this is error prone and not very ergonomic. It's also not simple to change this:
> 
> - The assertion handler needs to have been declared before any libc++ code is included. Otherwise, the compiler will complain that we are using `::HelloWorldAssertionHandler` in the code but it hasn't been declared yet.
> 
> - The assertion handler needs to be aware of the type `std::__libcpp_assertion_info`, which means that the declaration of `HelloWorldAssertionHandler` must come **after** including `<__assert>`, and **before** including anything else. That's why we have this weird include sandwich.
> 
> I think I can get rid of the second requirement by passing primitive types to the assertion handler, i.e. passing `char const* file, int line, char const* predicate, char const* message)` directly to the function. However, that prevents me from being backwards compatible with users that might have been setting `__libcpp_debug_function` (because I use the conversion operator in `__libcpp_assertion_info` to achieve that).
> 
> I'm not sure how to solve the first problem, though. In practice, I would expect that people use `-include <path-to-header-that-declares-the-assertion-handler>`.
> 
> To make progress, would you be OK with me changing the documentation to:
> 
> ```
> // In HelloWorldAssert.h
> #define _LIBCPP_ASSERTION_HANDLER(info) ::HelloWorldAssertionHandler(info)
> #include <__assert>
> 
> // This could also be defined out-of-line, but we're defining it inline
> // for simplicity in this example.
> inline void HelloWorldAssertionHandler(std::__libcpp_assertion_info info) {
>   std::printf("Assertion %s failed at %s:%d, more info: %s",
>               info.__predicate(), info.file(), info.__line(), info.__message());
>   std::abort();
> }
> 
> // In HelloWorld.cpp
> // Make sure to pass `-include path/to/HelloWorldAssert.h` to the compiler
> #include <vector>
> 
> int main() {
>   std::vector<int> v;
>   int& x = v[0]; // Your assertion handler will be called here if _LIBCPP_ENABLE_ASSERTIONS=1
> }
> ```
> 
> If we decide to break backwards compatibility with the debug mode, we can simplify this to (I'm happy to do it, but I would prefer doing it separately since it's a breaking change):
> 
> ```
> // In HelloWorldAssert.h
> #define _LIBCPP_ASSERTION_HANDLER(info) ::HelloWorldAssertionHandler(info)
> // This could also be defined out-of-line, but we're defining it inline
> // for simplicity in this example.
> inline void HelloWorldAssertionHandler(char const* file, int line, char const* expression, char const* message) {
>   std::printf("Assertion %s failed at %s:%d, more info: %s", expression, file, line, message);
>   std::abort();
> }
> 
> // Same story from HelloWorld.cpp, still need `-include path/to/HelloWorldAssert.h`
> ```
Ah, I see that I did forget something: you can't //just// `-D_LIBCPP_ASSERTION_HANDLER=foo` and then include libc++ headers, because you //also// need to provide a forward declaration of `foo` (otherwise it's an undeclared identifier). Gross.
(Backstory: My previous experiences with defining handlers for things via macros is all from https://github.com/troydhanson/uthash , where the handlers are used only //within macro expansions themselves//, and thus forward declarations are never needed; you just have to declare `foo` sometime before the first //call// to `HASH_ADD`-or-whatever.)

In absolute terms I'm not a huge fan of `-include` either... but yeah, I think putting `-include HelloWorld.h` into your build flags is a big improvement over putting forward declarations and stuff above the libc++ headers in every translation unit. :)


================
Comment at: libcxx/docs/UsingLibcxx.rst:167
+  #define _LIBCPP_ASSERTION_HANDLER(info) ::HelloWorldAssertionHandler(info)
+  #include <__assert>
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121123