[libcxx-commits] [PATCH] D89353: Enable overriding `__libcpp_debug_function` invocation

Wed May 19 18:03:26 PDT 2021

thakis added inline comments.

================
Comment at: libcxx/include/__debug:42
 #   define _LIBCPP_DEBUG_ASSERT(x, m) _LIBCPP_ASSERT(x, m)
-#   define _LIBCPP_ASSERT_IMPL(x, m) ((x) ? (void)0 : _VSTD::__libcpp_debug_function(_VSTD::__libcpp_debug_info(__FILE__, __LINE__, #x, m)))
+#   define _LIBCPP_ASSERT_IMPL(x, m) ((x) ? (void)0 : _LIBCPP_DEBUG_FUNCTION_INVOCATION((x), m))
 #else
----------------
Quuxplusone wrote:
> (A) Please use `#x` here instead of `(x)` (and then `x` instead of `#x` in the macro above). This preserves the current assertion messages without extra parens. I don't think there's any reason it's a big deal; but we have a choice between "add an extra set of parens" or "don't", and I think "don't" is clearly if microscopically better.
> 
> (B) Even after reading D70343, I don't get the point of this new abstraction. Is it so that you can hard-code `#define _LIBCPP_DEBUG_FUNCTION_INVOCATION std::abort()` at compile time instead of setting `__libcpp_debug_function = std::abort;` at runtime? Why is the latter unacceptable to you?
> Also, are you aware that libc++ compiles some `_LIBCPP_ASSERT` checks into its libc++.a binary? — Failures in those checks will respect the runtime setting of `__libcpp_debug_function`, but they will //not// respect macros set in the user's code. (I sent an email to libcxx-dev about this issue, specifically that debug iterators are broken because of it, on 2021-05-01, but nobody's replying to my email.)
> 
> (C) If you want libc++ to support this abstraction long-term, you absolutely need to "put a test on it." libcxx/test/libcxx/debug/ would probably be the right place for a new test that verifies the behavior of defining `_LIBCPP_DEBUG_FUNCTION_INVOCATION`.
(For B: We build libc++ from source and statically link it, so that's fine for us.)

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

https://reviews.llvm.org/D89353