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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

Thanks a lot! I think this is now a no-brainer since we've made this more conservative (it only triggers on a few comparators that we know are going to benefit from it).

Please ship this once you've addressed my comments and the CI is passing. If you need help committing this, please provide `Author Name <email at domain>` and we can commit it on your behalf. Thanks again for the patch and for your patience+collaboration going through the review!



================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/sort.pass.cpp:52-56
+template<> void set_value(std::pair<int, int>& dest, int value)
+{
+    dest.first = value;
+    dest.second = value;
+}
----------------
Instead of specializing the function template, can you instead add an overload?

```
inline void set_value(std::pair<int, int>& dest, int value)
{
    dest.first = value;
    dest.second = value;
}
```


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/sort.pass.cpp:74
 
-template <class RI>
+template <class _Container, class RI>
 void
----------------
Please don't use reserved identifiers in the tests. This should be `class Constainer` instead of `class _Container`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/sort.pass.cpp:98
+
+template<> std::pair<int, int> increment_or_reset(std::pair<int, int> value,
+                                                  int max_value)
----------------
Same comment, overload instead of specialization please :-)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/sort.pass.cpp:113
 {
+    using _Iter = typename _Container::iterator;
+    using _Tp = typename _Container::value_type;
----------------
Should be `Iter`, not `_Iter`.

We only use `_Foo` and `__foo` in the source code of the library itself because it ensures that no users can possibly define a macro with a name that conflicts with our identifiers. Inside our test code, this isn't needed (or desirable since it obscures everything).


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/sort.pass.cpp:137
+    assert(std::is_sorted(iter, iter+N));
+    assert(std::is_permutation(iter, iter+N, original_iter));
     // test sorted pattern
----------------
I like that we're adding this coverage!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118029