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

[Nikolas Klauser via Phabricator via llvm-commits]

philnik 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>
----------------
IMO we shouldn't include stuff based on whether we are in debug mode or not.


================
Comment at: libcxx/include/__debug_utils/strict_weak_ordering_check.h:28
+
+template <class _AlgPolicy, class _RandomAccessIterator, class _Comp>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14
----------------
If you never use the `_AlgPolicy` there is no point in passing it.


================
Comment at: libcxx/include/__debug_utils/strict_weak_ordering_check.h:32
+#ifdef _LIBCPP_DEBUG_STRICT_WEAK_ORDERING_CHECK
+  typedef typename iterator_traits<_RandomAccessIterator>::difference_type difference_type;
+  using _Comp_ref = __comp_ref_type<_Comp>;
----------------



================
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");
----------------
Could we print the indices/types if they can be printed instead of just saying "you are wrong"?


================
Comment at: libcxx/include/module.modulemap.in:1117-1118
   module __debug_utils {
     module randomize_range { private header "__debug_utils/randomize_range.h" }
+    module strict_weak_ordering_check { private header "__debug_utils/strict_weak_ordering_check.h" }
   }
----------------



================
Comment at: libcxx/test/libcxx/algorithms/alg.sorting/assert.sort.invalid_comparator.pass.cpp:172
+// Nans in floats do not satisfy strict weak ordering by breaking transitivity of equivalence.
+std::vector<float> generate_float_data() {
+    std::vector<float> floats(50);
----------------
Could you use a struct which violates the condition instead? floating-point types specifically are almost trivial to diagnose at compile-time. A struct which uses a float should be hard enough to diagnose (at least from the library).


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/sort.heap/complexity.pass.cpp:72
     LIBCPP_ASSERT(std::is_sorted(first, last));
-    LIBCPP_ASSERT(stats.compared <= 2 * n * logn);
   }
----------------
Could this be extended by a constant instead?


================
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});
   }
----------------
Why is this changed?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/sort.heap/ranges_sort_heap.pass.cpp:266
     LIBCPP_ASSERT(std::is_sorted(first, last, &MyInt::Comp));
-    LIBCPP_ASSERT(stats.compared <= 2 * n * logn);
   }
----------------
Could this be extended by a constant instead?


================
Comment at: libcxx/utils/data/ignore_format.txt:308
 libcxx/include/__debug_utils/randomize_range.h
+libcxx/include/__debug_utils/strict_weak_ordering_check.h
 libcxx/include/deque
----------------
Please clang-format the header.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150264