[libcxx-commits] [PATCH] D128618: [libc++] Implement ranges::remove{, _if}

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_remove_if.h:36
+_LIBCPP_HIDE_FROM_ABI constexpr
+subrange<_Iter> __remove_if_impl(_Iter __first, _Sent __last, _Pred& __pred, _Proj& __proj) {
+  auto __end = ranges::__find_if_impl(__first, __last, __pred, __proj);
----------------
huixie90 wrote:
> By having an impl function, you can save 2 moves of `pred` and `proj` of the range overload, at the cost of having 2 extra moves of iterator/sentinel for iterator overload. is it worth it? sometimes moving iterators can be expensive, especially for iterators in the range adaptor (e.g. zip iterator)
I'd keep it for now as-is, but I think we want to review the stance on moving the iterators. I don't think anybody had `zip_view` and friends in mind when we decided to take the iterators by value.


================
Comment at: libcxx/include/__algorithm/ranges_remove_if.h:37
+subrange<_Iter> __remove_if_impl(_Iter __first, _Sent __last, _Pred& __pred, _Proj& __proj) {
+  auto __end = ranges::__find_if_impl(__first, __last, __pred, __proj);
+  if (__end == __last)
----------------
var-const wrote:
> I found the name `__end` a little confusing -- it's not obvious how it is different from `__last`. How about `new_end` or similar?
I think `__new_end` could also be a bit confusing, but definitely better than `__end`.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp:53
+static_assert(!HasRemoveR<SentinelForNotSemiregular>);
+static_assert(!HasRemoveR<SentinelForNotWeaklyEqualityComparableWith>);
+static_assert(!HasRemoveR<UncheckedRange<int**>>); // not indirect_unary_predicate
----------------
var-const wrote:
> Please also check `permutable`.
That's done in lines 50 and 51, or am I missing something?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp:65
+constexpr void test(Data<N, M> d) {
+  {
+    auto input = d.input;
----------------
var-const wrote:
> Please add brief comments to explain what is being tested.
What exactly are you asking for? Something like `check that the expected outcome is the actual outcome` isn't exactly useful, but I don't think there is anything more specific I could say.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128618