[libcxx-commits] [PATCH] D125634: [libc++] Use _LIBCPP_ASSERT by default for _PSTL_ASSERTions

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked 2 inline comments as done.
philnik added inline comments.


================
Comment at: pstl/include/pstl/internal/pstl_config.h:33-34
+#    define _PSTL_ASSERT(pred) _LIBCPP_ASSERT(pred, "")
+#elif defined(__GLIBCXX__)
+#    define _PSTL_ASSERT(pred) __glibcxx_assert(pred)
+#else
----------------
jwakely wrote:
> ldionne wrote:
> > @jwakely @rodgert We'd like to remove `_PSTL_ASSERT` as a customization point and define it explicitly here instead, are you OK with that?
> Makes sense to me in principle, although I think `__glibcxx_assert` is the wrong definition (even though that's what we use today).
> 
> I see several assertions in `parallel_backend_tbb.h` like `_PSTL_ASSERT(std::is_sorted(_M_x_beg + _M_xs, _M_x_beg + _M_xe, _M_comp));` which changes the complexity guarantees of the algo. That's not appropriate for `__glibcxx_assert`, but would be appropriate for the more heavyweight `_GLIBCXX_DEBUG_ASSERT`.
> 
> Maybe that should be fixed as a separate issue, as I assume that isn't appropriate for libc++ either.
> 
> We could introduce `_PSTL_DEBUG_ASSERT` for the more expensive assertions, and map that to `_LIBCPP_DEBUG_ASSERT` or `_GLIBCXX_DEBUG_ASSERT`.
I'm going to take this as a yes. I think adding `_PSTL_DEBUG_ASSERT` or something similar should be deferred to after having added a CI configuration with the PSTL enabled, so we actually check it instead of hoping that it works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125634