[libcxx-commits] [PATCH] D140824: [libc++] Forward ranges::sort to instantiations in the dylib

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I generally like this change, but I think we want to clean up the situation with `__sort5` and friends before we do it.



================
Comment at: libcxx/src/algorithm.cpp:13-16
+template <class Comp, class RandomAccessIterator>
+void __sort(RandomAccessIterator first, RandomAccessIterator last, Comp comp) {
+  std::__sort_dispatch<_ClassicAlgPolicy, RandomAccessIterator, Comp>(first, last, comp);
+}
----------------
Basically, don't rely on the (way too subtle) trick where you're avoiding deadly recursion because you're passing the third template argument to `__sort`.


================
Comment at: libcxx/src/algorithm.cpp:36
 
-template bool __insertion_sort_incomplete<__less<char>&, char*>(char*, char*, __less<char>&);
+// TODO: Were the __insertion_sort_incomplete and __sort5 specializations ever visible?
+
----------------
I don't think these symbols could actually be called previously, because they are only used from `__sort`, which is in the dylib as well.

I would actually start with that patch since it'll simplify the rest of this patch. I would not ship this in LLVM 16 though, to give us a bit of time to find potential issues with it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140824