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

[Nilay Vaish via Phabricator via llvm-commits]

nilayvaish added inline comments.


================
Comment at: libcxx/src/legacy-sort.cpp:339-393
+template _LIBCPP_FUNC_VIS void __sort<__less<char>&, char*>(char*, char*, __less<char>&);
+template _LIBCPP_FUNC_VIS void __sort<__less<wchar_t>&, wchar_t*>(wchar_t*, wchar_t*, __less<wchar_t>&);
+template _LIBCPP_FUNC_VIS void __sort<__less<signed char>&, signed char*>(signed char*, signed char*,
+                                                                          __less<signed char>&);
+template _LIBCPP_FUNC_VIS void __sort<__less<unsigned char>&, unsigned char*>(unsigned char*, unsigned char*,
+                                                                              __less<unsigned char>&);
+template _LIBCPP_FUNC_VIS void __sort<__less<short>&, short*>(short*, short*, __less<short>&);
----------------
ldionne wrote:
> nilayvaish wrote:
> > ldionne@, I am wondering if these symbols need to be in this file.  Can we continue with the setup from before i.e. these symbols have extern declarations in sort.h and are defined in algorithm.cpp?  Further is there a need for retaining the existing sorting algorithm?  It seems to me we need to retain __insertion_sort_incomplete only.  What do you think?
> I think we do need to retain all those functions since they were previously exported from the shared library. Removing these functions would be an ABI break.
> 
> I've put it in `legacy-sort.cpp` because we don't ever want to deal with these functions again anymore -- they are only there for ABI compatibility purpose. I thought it was better to separate them into their own little file than keeping them around in the headers that we actually use. If you have a strong reason to keep them around in `sort.h`, let me know and we can discuss.
I do not have a preference for where these symbols are kept.  Is there a way to avoid having sort implementation in legacy-sort.cpp file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93233