[libcxx-commits] [PATCH] D109086: [libc++] Implement ranges::filter_view

Fri Apr 1 16:58:56 PDT 2022

var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.

Very close to LGTM. Most comments are optional (which includes all nits), but there's just a couple that I think are worth addressing.

================
Comment at: libcxx/include/__ranges/filter_view.h:48
+  class filter_view : public view_interface<filter_view<_View, _Pred>> {
+    _LIBCPP_NO_UNIQUE_ADDRESS __copyable_box<_Pred> __pred_;
+    _LIBCPP_NO_UNIQUE_ADDRESS _View __base_ = _View();
----------------
Nit: is there a reason to swap the order these are declared in, compared to the Standard? Not that it's an issue, but because of this, the constructor taking both of these parameters looks a little strange because the initialization order is the reverse of the argument order. Also, if there's a reason, I think it should be documented?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/adaptor.pass.cpp:65
+constexpr bool test() {
+  int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
+
----------------
Nit/question: why not let the number of elements to be deduced?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/begin.pass.cpp:154
+    struct MutablePred {
+      constexpr bool operator()(int i) { return i % 2 == 0; }
+    };
----------------
Question: would it be worthwhile to test with a predicate that takes the argument by non-const reference?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/ctad.pass.cpp:27
+
+// A range, but not a view
+struct Range {
----------------
Very optional: consider turning this comment into a static assertion instead.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/ctor.default.pass.cpp:13
+// filter_view() requires std::default_initializable<View> &&
+//                        std::default_initializable<Pred> = default;
+
----------------
Same comment about potentially testing `noexcept` as another default constructor test.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/ctor.view_pred.pass.cpp:78
+    Pred pred;
+    std::ranges::filter_view<TrackingRange, Pred> view(std::move(range), pred); (void)view;
+    assert(moved);
----------------
Nit: consider using `[[maybe_unused]]`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/iterator/compare.pass.cpp:13
+// friend constexpr bool operator==(iterator const&, iterator const&)
+//  requires equality_comparable<iterator_t<V>>
+
----------------
Optional: this seems untested.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/iterator/compare.pass.cpp:35
+constexpr void test() {
+  using View = minimal_view<Iterator, Sentinel>;
+  using FilterView = std::ranges::filter_view<View, AlwaysTrue>;
----------------
Optional: perhaps these type aliases could be moved to `types.h` as well, along with `make_filter_view`. They seem to take a significant part of the setup in many of these test files.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/iterator/compare.pass.cpp:73
+  {
+    static_assert(!has_equal<FilterIteratorFor<cpp20_input_iterator<int*>>>);
+  }
----------------
Nit: move this right under the definition of `FilterIteratorFor` (I think it would be easier to read when the definition and the usage are next to each other)?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/iterator/deref.pass.cpp:35
+
+  std::array<int, 10> array{0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
+  FilterView view = make_filter_view(array.begin(), array.end(), AlwaysTrue{});
----------------
Very optional: any reason not to get CTAD to deduce the number of arguments? The element type seems obvious enough not to cause any issues.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/iterator/increment.pass.cpp:140
+  tests();
+  // static_assert(tests());
+  return 0;
----------------
Looks like this is either a leftover, or some issue that requires a comment.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/iterator/iter_move.pass.cpp:13
+// friend constexpr range_rvalue_reference_t<V> iter_move(iterator const& i)
+//  noexcept(noexcept(ranges::iter_move(i.current_)));
+
----------------
`noexcept` doesn't seem to be tested.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/iterator/iter_move.pass.cpp:56
+
+int main(int, char**) {
+  tests();
----------------
Same comments as `iter_swap.pass.cpp`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/iterator/iter_swap.pass.cpp:14
+//  noexcept(noexcept(ranges::iter_swap(x.current_, y.current_)))
+//  requires(indirectly_swappable<iterator_t<V>>);
+
----------------
I don't see a test for this constraint. If you think it's overkill, feel free to ignore.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/iterator/iter_swap.pass.cpp:36
+
+  auto make_filter_view = [](auto begin, auto end, auto pred) {
+    View view{Iterator(begin), Sentinel(Iterator(end))};
----------------
Optional: this function is duplicated in many of the test files, I wonder if it should be moved to `types.h`?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/iterator/iter_swap.pass.cpp:50
+
+    assert(*it1 == 2 && *it2 == 4); // test the test
+    iter_swap(it1, it2);
----------------
Very optional: consider splitting this into two assertions (to make it easier to see which condition failed if debugging is ever necessary). Applies below as well.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/iterator/iter_swap.pass.cpp:51
+    assert(*it1 == 2 && *it2 == 4); // test the test
+    iter_swap(it1, it2);
+    assert(*it1 == 4 && *it2 == 2);
----------------
I don't think this test file checks that `iter_swap` uses `ranges::iter_swap` (which a user can customize). Do you think it's worthwhile to test for that? Same with `iter_move`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/iterator/types.compile.pass.cpp:66
+
+  // Check iterator_category for various categories of ranges
+  {
----------------
Hmm, I don't see a test for
```
3.4 Otherwise, iterator_category denotes C.
```

Would it be worthwhile to check with e.g. `random_access_iterator_tag`?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/sentinel/base.pass.cpp:30
+    View view{Iterator(begin), Sentinel(Iterator(end))};
+    return FilterView(std::move(view), pred);
+  };
----------------
Ultranit: include `<utility>` (applies in a few test files).

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/sentinel/base.pass.cpp:37
+  FilterSentinel const sent = view.end();
+  std::same_as<Sentinel> auto result = sent.base();
+  assert(base(base(result)) == array.end());
----------------
Nit: should this (and other similar uses) be `decltype(auto)` in the light of our recent discussion?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/sentinel/compare.pass.cpp:40
+  std::same_as<bool> auto result = (it == sent);
+  assert(!result);
+}
----------------
Optional: test the case where `result` is true as well?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/sentinel/ctor.default.pass.cpp:12
+
+// filter_view<V>::<sentinel>() = default;
+
----------------
Optional: would it be worthwhile testing for noexcept-edness?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/sentinel/ctor.parent.pass.cpp:12
+
+// constexpr explicit sentinel(filter_view&);
+
----------------
Optional: test that the constructor is `explicit`?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109086