[PATCH] D93233: [libc++] Replaces std::sort by Bitset sorting algorithm.

Tue Nov 9 07:03:17 PST 2021

ldionne added a comment.

In D93233#3101571 <https://reviews.llvm.org/D93233#3101571>, @nilayvaish wrote:

> Hi ldionne@, I have question for you.  Do the tests we have for libc++ ensure that the new implementation works well across all versions of the C++ standard?  I want to make sure that I am not going to unintentionally break someone's standard-compliant code out there.

They should, however you're welcome to look at them - our tests are sometimes not up to our quality standards, so you should make sure they will catch all issues you can think about.

We do run those tests in C++03, C++11, C++14, C++17, C++20 and C++23 modes, if that was the sole intent of your question.

In D93233#3116956 <https://reviews.llvm.org/D93233#3116956>, @nilayvaish wrote:

> ldionne@, I am going to break this change into few parts and provide the benchmark results for each of the part separately.  To begin with, I have posted: https://reviews.llvm.org/D113413 that makes the introsort change to the std::sort implementation.

That's fine by me if you prefer doing it that way, however at the moment I am failing to understand where https://reviews.llvm.org/D113413 fits into the picture. Can you explain what's the plan? Basically introduce introsort in D113413 <https://reviews.llvm.org/D113413> and then replace it by this bitset sort? Sorry if that's a stupid question.

================
Comment at: libcxx/include/__algorithm/sort.h:34

-// stable, 2-3 compares, 0-2 swaps
+namespace __sorting_network {

----------------
nilayvaish wrote:
> ldionne wrote:
> > Why is this called `__sorting_network`? Sounds like a weird name.
> I think the name has been prevalent in the theory community for a long time.  More info here: https://en.wikipedia.org/wiki/Sorting_network.  The code in functions __sort[3...8] mimics what the sorting networks for those lengths would look like.  The name is strange from namespace perspective.  Willing to name it something else that you prefer.
Oh, that's all good. If there's prior art with this name, ignore my comment -- I was not aware of it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93233