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

Mon Apr 11 11:12:24 PDT 2022

ldionne added inline comments.

================
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();
----------------
var-const wrote:
> 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?
I believe this was originally to work around https://wg21.link/LWG3549 where we'd have padding if the base view inherited from `view_interface`, but that shouldn't be an issue anymore, so I flipped them.

================
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};
+
----------------
var-const wrote:
> Nit/question: why not let the number of elements to be deduced?
It is probably a remnant of copy-pasting tests for other adaptors. Changed.

================
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; }
+    };
----------------
var-const wrote:
> Question: would it be worthwhile to test with a predicate that takes the argument by non-const reference?
Test added. IMO it's a bit borderline on testing the implementation of `ranges::find_if` itself, but definitely easy enough to add. I added the same test to `iterator::operator++` too since it calls `ranges::find_if` as well.

================
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);
----------------
var-const wrote:
> Nit: consider using `[[maybe_unused]]`.
Good point, applied throughout. I don't have the reflex to use it but I should. I guess that makes me old school.

================
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>>
+
----------------
var-const wrote:
> Optional: this seems untested.
It is tested below with the `static_assert(!has_equal<...>);`

================
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>;
----------------
var-const wrote:
> 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.
Unfortunately, those are often slightly different in different test files, so I think this would add complexity instead of simplifying things.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/iterator/compare.pass.cpp:73
+  {
+    static_assert(!has_equal<FilterIteratorFor<cpp20_input_iterator<int*>>>);
+  }
----------------
var-const wrote:
> 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)?
I'll actually remove `FilterIteratorFor` from this file.

================
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))};
----------------
var-const wrote:
> Optional: this function is duplicated in many of the test files, I wonder if it should be moved to `types.h`?
Hmm, you're right about the duplication, however the type of the view is hardcoded here in this test. If I move it to a generic place like `types.h`, I'd have to hardcode the fact that it returns a `filter_view` over a `minimal_view`, and IMO that would make it pretty weird. So yeah, I'm not sure it's worth sharing.

================
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);
----------------
var-const wrote:
> 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.
IMO it makes sense to split the checks below because they are actual things that we're testing, however this one is just testing the test, so I think it makes more sense to leave it as terse an unobtrusive as possible.

================
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);
----------------
var-const wrote:
> 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`.
Added tests by defining my own noexcept/non-noexcept types that override `iter_move()` (here and in `iter_move`).

================
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());
----------------
var-const wrote:
> Nit: should this (and other similar uses) be `decltype(auto)` in the light of our recent discussion?
I'm using `ASSERT_SAME_TYPE` in a bunch of places because of the issues with `std::same_as<...> auto`, but I used `decltype(auto)` in the few remaining places.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.filter/sentinel/ctor.parent.pass.cpp:12
+
+// constexpr explicit sentinel(filter_view&);
+
----------------
var-const wrote:
> Optional: test that the constructor is `explicit`?
There is one below that checks `!std::is_convertible`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109086