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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
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);
   }
----------------
huixie90 wrote:
> 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?
That is an interesting corner case, however that would also require the range's iterator to be a `random_access_iterator` but not a `sized_sentinel_for<It, Sent>`. That seems rather broken to me.

For example, imagine a null-terminated string where the iterators are random access (roughly `char*`), but comparing against the end sentinel checks whether `*it == '\0'`. If the range is a `sized_range`, it means that it also stores the size of the string. In a case like that, I can't imagine why one would implement the iterator and the sentinel as different types, because the size is known anyway so `end()` can be implemented as `begin() + size()` trivially.

If I'm not imaginative enough, please feel free to enlighten me, however if this is such a tiny corner case, perhaps it is not worth increasing the complexity of our implementation to handle it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130547