[libcxx-commits] [PATCH] D130547: [libc++][ranges] Implement `ranges::is_heap{, _until}`.

[Hui via Phabricator via libcxx-commits]

huixie90 accepted this revision as: huixie90.
huixie90 added a comment.

Almost LGTM thanks!



================
Comment at: libcxx/include/__algorithm/is_heap.h:31
     typedef typename __comp_ref_type<_Compare>::type _Comp_ref;
-    return _VSTD::__is_heap_until<_Comp_ref>(__first, __last, __comp) == __last;
+    return std::__is_heap_until<_Comp_ref>(__first, __last, static_cast<_Comp_ref>(__comp)) == __last;
 }
----------------
i think perhaps not to pass the comp_ref type explicitly since the `__is_heap_until` already takes it by reference? 


================
Comment at: libcxx/include/__algorithm/ranges_is_heap.h:59
   bool operator()(_Range&& __range, _Comp __comp = {}, _Proj __proj = {}) const {
-    // TODO: implement
-    (void)__range; (void)__comp; (void)__proj;
-    return {};
+    return __is_heap_fn_impl(ranges::begin(__range), ranges::end(__range), __comp, __proj);
   }
----------------
nit:
for ranges that do model `sized_range` but their iter/sent pair do not model `sized_sentinel_for`, by doing this it lost its size information and possibly as a result the `ranges::next` call becomes linear whereas it could be constant.  What do you think?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/is.heap/ranges_is_heap.pass.cpp:147-148
+
+  { // A custom projection works.
+    std::array in = {-8, -6, -7, -3, -4, -1, -5, -2};
+    auto negate = [](int x) { return x * -1; };
----------------
most other tests use a different type to test projection. perhaps it is a good idea to adopt the same approach, since having different types sound more realistic use case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130547