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

Mon Dec 14 11:15:12 PST 2020

ldionne added a comment.

This is an interesting contribution. I'm trying to wrap my head around it.

In D93233#2452718 <https://reviews.llvm.org/D93233#2452718>, @MaskRay wrote:

> From a glance this uses a bitset partition and needs temporary storage (heap memory allocation?).  Does the original algorithm have heap memory allocation?

Unless I missed a subtlety, it doesn't require heap allocation. The "bitsets" used are actually 64 (or 32) bit integers. The current algorithm also doesn't have heap allocation, so keeping that property is something we want (and something this patch does AFAICT).

@minjaehwang Is there documentation for this algorithm? I believe it would help us (at least myself) understand the algorithm and the potential tradeoffs involved. Also, are you aware of Timsort? IIRC there had been an analysis conducted that concluded that we should be using Timsort (as it was faster than our sort and respected the complexity requirements of the Standard).

================
Comment at: libcxx/test/libcxx/algorithms/sort.pass.cpp:21
+#ifndef _LIBCPP_VERSION
+#error _LIBCPP_VERSION not defined
+#endif
----------------
Shouldn't this test work with all implementations? Why not?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93233