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

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/docs/DesignDocs/DebugMode.rst:113
   * ``std::list``
+  * ``std::locale``
   * ``std::unordered_map``
----------------
In what sense does `std::locale` support iterators?


================
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
----------------
(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`.


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

https://reviews.llvm.org/D89353