[libcxx-commits] [PATCH] D150264: [libcxx] Add strict weak ordering checks to sorting algorithms

[Danila Kutenin via Phabricator via libcxx-commits]

danlark added inline comments.


================
Comment at: libcxx/include/__debug_utils/strict_weak_ordering_check.h:14
+
+#ifdef _LIBCPP_DEBUG_STRICT_WEAK_ORDERING_CHECK
+#  include <__algorithm/comp_ref_type.h>
----------------
philnik wrote:
> IMO we shouldn't include stuff based on whether we are in debug mode or not.
This is consistent with __debug_utils/randomize_range. I believe the intention is not to expose more headers than needed for compile times


================
Comment at: libcxx/include/__debug_utils/strict_weak_ordering_check.h:49
+        for (difference_type __a = __p; __a <= __b; ++__a) {
+          _LIBCPP_ASSERT(!__comp(*(__first + __a), *(__first + __b)), "Your comparator is not a valid strict-weak ordering");
+          _LIBCPP_ASSERT(!__comp(*(__first + __b), *(__first + __a)), "Your comparator is not a valid strict-weak ordering");
----------------
philnik wrote:
> Could we print the indices/types if they can be printed instead of just saying "you are wrong"?
This information might be incorrect. As said in the RFC, quadratic algorithm can show if there is a problem, not the triple that violates the condition

In this case we figured out a pair in the range that we think consist of equal elements to be comparable, say, it can be with the epsilon float sorting (2 elements are considered equal if their diff is no more than eps)

eps/2, eps, 2eps

We will detect a pair eps/2, 2eps and we will show it to the user. It does not explain much on what happened.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/sort.heap/ranges_sort_heap.pass.cpp:210
     [[maybe_unused]] std::same_as<std::ranges::dangling> decltype(auto) result =
-        std::ranges::sort_heap(std::array{2, 1, 3});
+        std::ranges::sort_heap(std::array{3, 1, 2});
   }
----------------
philnik wrote:
> Why is this changed?
Because 2, 1, 3 is not a max heap, calling sort_heap is incorrect in this case


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

https://reviews.llvm.org/D150264