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

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

Quuxplusone added a comment.

FWIW, I have no strong desire to be obstructionist, and I definitely don't think we should //abandon// this direction, but I have one bad and one good(?) reason I think we should hold this PR in suspended animation for a while longer:

- Bad: I'm still uncomfortable with the invasiveness of the changes to `robust_against_copying_comparators.pass.cpp`.
- Good(?): Very soon now we're going to have to implement `ranges::sort`, which innovates in a bunch of different directions — we're going to have to deal with sentinels as well as iterators, and we're going to have to replace `__c(*x, *y)` with `invoke(__c, invoke(__p, *x), invoke(__p, *y)`, and so on. I feel like it would be a good idea to get //that// stuff out of the way first, and then revisit algorithmic changes like this PR, rather than vice versa. Unfortunately I have no particular estimate of when `ranges::sort` will get done; @philnik might have a long-range plan to share, but being as we're still down in the foothills of `ranges::min_element` etc., personally I'd predict it'll be a couple months //at least//.

Anyway, @ldionne will be the ultimate arbiter (but IIRC he's on vacation this week).

I'll also keep recommending that you replace `is_arithmetic` with `is_scalar`, until you actually do it! or explain why not. :)



================
Comment at: libcxx/benchmarks/algorithms.bench.cpp:30-31
             V() == ValueType::Pair, std::pair<uint32_t, uint32_t>,
-            std::conditional_t<V() == ValueType::Tuple,
-                               std::tuple<uint32_t, uint64_t, uint32_t>,
-                               std::string> > > >;
+            std::conditional_t< V() == ValueType::Tuple, std::tuple<uint32_t, uint64_t, uint32_t>,
+                                std::conditional_t< V() == ValueType::String, std::string, float> > > > >;
 
----------------
marcogelmi wrote:
> Quuxplusone wrote:
> > philnik wrote:
> > > 
> > It might be (long past) time to change this to simply
> > ```
> > using Types = std::tuple<
> >     uint32_t, uint64_t, std::pair<uint32_t, uint64_t>, std::tuple<uint32_t, uint64_t, uint32_t>,
> >     std::string, float
> > >;
> > 
> > template<class V>
> > using Value = std::tuple_element_t<(int)V(), Types>;
> > ```
> note that I'm using clang-format and it's changing the format slightly. Also, I had to change to a plain enum
For my curiosity, why did you have to change to a plain enum? (All else being equal, we should prefer `enum class`, but if it really technically doesn't work, then I don't imagine it's a big deal.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118029