[libcxx-commits] [PATCH] D130627: [libc++][ranges] implement `std::ranges::inplace_merge`

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.

Thanks a lot! A few nitpicks, and let's discuss `algorithm_family.h` offline (I'll post a summary here afterwards).



================
Comment at: libcxx/docs/Status/RangesAlgorithms.csv:80
 Permutation,partial_sort,Konstantin Varlamov,`D128744 <https://llvm.org/D128744>`_,✅
-Permutation,inplace_merge,Not assigned,n/a,Not started
+Permutation,inplace_merge,Hui Xie,`D130404 <https://llvm.org/D130404>`_,✅
 Permutation,make_heap,Konstantin Varlamov,`D128115 <https://llvm.org/D128115>`_,✅
----------------
The patch number needs to be updated.


================
Comment at: libcxx/include/__algorithm/inplace_merge.h:50
+  template <class _T1>
+  _LIBCPP_HIDE_FROM_ABI bool operator()(const _T1& __x) {
+    return !__p_(__x);
----------------
Nit: it's best not to do these refactoring changes in this patch (for the lines that you don't otherwise modify). It increases the delta that needs to be reviewed, but more importantly, it makes it harder to navigate `git blame`. Ideally, such changes should go to dedicated refactoring patches which can then be added to `.git-blame-ignore-revs`.

This doesn't apply to the lines that you're modifying anyway -- since those would show up in `git blame` regardless, might as well refactor them.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_inplace_merge.pass.cpp:92
+template <class In, template <class> class SentWrapper, std::size_t N1, std::size_t N2>
+void testInplaceMergeImpl(std::array<int, N1> input, int midIdx, std::array<int, N2> expected) {
+  using Sent = SentWrapper<In>;
----------------
Optional: consider asserting that `(input.begin(), input.begin() + midIdx)` and `(input.begin() + midIdx, input.end())` are `is_sorted`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_inplace_merge.pass.cpp:123
+
+  //  is longer
+  {
----------------
Missing `[first, mid)`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_inplace_merge.pass.cpp:144
+
+  // duplictes within each range
+  {
----------------
Nit: `s/duplictes/duplicates/`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_inplace_merge.pass.cpp:165
+
+  // [mid, last] is empty (mid == end)
+  {
----------------
Consider also checking `mid == first + 1` and `mid == last - 1` (to check for off-by-one errors).


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_inplace_merge.pass.cpp:273
+
+  // Complexity
+  {
----------------
Please copy the specification from the standard into this comment.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_inplace_merge.pass.cpp:292
+
+      // the spec specify exactly N-1 comparison but we actually
+      // does not invoke as many times as specified
----------------
Ultranits: `s/specify/specifies/` and `s/does not/don't/`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_inplace_merge.pass.cpp:292
+
+      // the spec specify exactly N-1 comparison but we actually
+      // does not invoke as many times as specified
----------------
var-const wrote:
> Ultranits: `s/specify/specifies/` and `s/does not/don't/`.
I noticed the same with `shuffle`. It could be a simple oversight in the spec.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_inplace_merge.pass.cpp:318
   test();
-  static_assert(test());
+  // inplace_merge is not constexpr (yet)
 
----------------
Nit: "yet" makes it a little ambiguous whether it's not `constexpr` in the standard or just in our implementation. I'd rephrase to `inplace_merge is not constexpr in the latest finished Standard`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130627