[libcxx-commits] [PATCH] D127557: [libc++][ranges] Implement `ranges::sort`.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this, a few comments.



================
Comment at: libcxx/include/__algorithm/sort.h:572
 
-template <class _RandomAccessIterator, class _Compare>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 void
-sort(_RandomAccessIterator __first, _RandomAccessIterator __last, _Compare __comp) {
+template <class _Iter, class _Comp>
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17 void
----------------
I think it makes sense to keep the `_RandomAccessIterator` name for documentation here.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/ranges.sort.pass.cpp:201
+    }
+  }
+
----------------
Please add a test for the complexity.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/ranges.sort.pass.cpp:18
+//   constexpr I
+//     sort(I first, S last, Comp comp = {}, Proj proj = {});                                // since C++20
+//
----------------
var-const wrote:
> philnik wrote:
> > I don't think we're adding this to the synopsis in tests, but I don't really care either way.
> FWIW, I usually do. I think copy-pasting is easiest (not just to write, but also to maintain) and there doesn't seem to be a strong reason to omit these annotations.
I usually don't see the `since` annotation in the synopsis here. However I don't mind so feel free to keep them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127557