[libcxx-commits] [PATCH] D118029: Introduce branchless sorting functions for sort3, sort4 and sort5.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__algorithm/sort.h:134
+  *__x = __tmp;
+}
+
----------------
Either call `_VSTD::__cond_swap<_Compare>(...)` below, or (less good IMHO) change it up here to take `_Compare&` instead of `_Compare`. Otherwise we end up making copies of the predicate — and I would have expected that to fail `libcxx/test/libcxx/algorithms/robust_against_copying_comparators.pass.cpp`! Could you also find out why that test //isn't// failing, and add a regression test to it? (I imagine it's just not taking the offending codepath because of the shape of the input data values, so adding a few extra test cases of the correct shape will solve the problem.)

Ditto `__partially_sorted_swap`.


================
Comment at: libcxx/include/__algorithm/sort.h:187-191
+template <typename _Compare, typename _RandomAccessIterator>
+inline _LIBCPP_HIDE_FROM_ABI
+    __enable_if_t<is_arithmetic<typename iterator_traits<_RandomAccessIterator>::value_type>::value, void>
+    __sort5_maybe_branchless(_RandomAccessIterator __x1, _RandomAccessIterator __x2, _RandomAccessIterator __x3,
+                             _RandomAccessIterator __x4, _RandomAccessIterator __x5, _Compare __c) {
----------------
Nit: It's more libc++ style to use `enable_if_t<>* = nullptr` than return type SFINAE; I'd say the advantages are readability and that return type SFINAE greatly increases the length of the mangled name while an extra `void*` type parameter doesn't so much.
Drive-by `s/typename/class/` and whitespace.
Likewise below.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118029