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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__algorithm/make_heap.h:42
 template <class _RandomAccessIterator, class _Compare>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
-void
-make_heap(_RandomAccessIterator __first, _RandomAccessIterator __last, _Compare __comp)
-{
-    typedef typename __comp_ref_type<_Compare>::type _Comp_ref;
-    _VSTD::__make_heap<_Comp_ref>(__first, __last, __comp);
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
+void make_heap(_RandomAccessIterator __first, _RandomAccessIterator __last, _Compare __comp) {
----------------
Mordante wrote:
> Maybe drop the `inline` while you're at it.
This seems like more than a purely mechanical change; I'd rather keep as is.


================
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);
 }
----------------
Mordante wrote:
> You need to calculate `__last - __first` before the function call since they are moved from in this call.
Thanks!


================
Comment at: libcxx/include/algorithm:290
 
+<<<<<<< HEAD
+=======
----------------
huixie90 wrote:
> unresolved merge conflicts?
Sorry about that!


================
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>);
----------------
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.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/make.heap/ranges_make_heap.pass.cpp:217
+  return true;
+}
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128115