[libcxx-commits] [PATCH] D128115: [libc++][ranges] Implement modifying heap algorithms:

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

A few comments other than that LGTM! But I like to see the CI green before approving.



================
Comment at: libcxx/include/__algorithm/push_heap.h:53
+  using _CompRef = typename __comp_ref_type<_Compare>::type;
+  std::__sift_up<_CompRef>(std::move(__first), std::move(__last), __comp, __last - __first);
 }
----------------
var-const wrote:
> Mordante wrote:
> > You need to calculate `__last - __first` before the function call since they are moved from in this call.
> Thanks!
I would prefer not to use `auto`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/make.heap/ranges_make_heap.pass.cpp:43
+
+static_assert(HasMakeHeapIt<int*>);
+static_assert(!HasMakeHeapIt<RandomAccessIteratorNotDerivedFrom>);
----------------
var-const wrote:
> Mordante wrote:
> > I think it would be good to test with all iterator categories to see only random access and contiguous are valid.
> This would apply to every test range algorithm test and make it more verbose without increasing coverage. The types from `almost_satisfies_types.h` are deliberately written to fall just short of meeting constraints of the type while satisfying all the prerequisite constraints; e.g. both `RandomAccessIteratorNotDerivedFrom` and `RandomAccessIteratorBadIndex` satisfy `bidirectional_iterator`. Thus, if `RandomAccessIteratorNotDerivedFrom` doesn't satisfy the constraints, it means that `bidirectional_iterator` and all the iterator concepts it refines don't satisfy, either.
Fair point.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/make.heap/ranges_make_heap.pass.cpp:217
+  return true;
+}
+
----------------
var-const wrote:
> Mordante wrote:
> > I really would like to see the maximum complexity to be validated for all heap operations. Especially since they all have different complexities.
> I think this is valuable but I'd rather do a follow-up after we finished the initial implementation of range algorithms dedicated solely to testing complexity where possible.
Should we add a note on the Status page as a reminder?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128115