[libcxx-commits] [PATCH] D107096: [libcxx][ranges] Add `ranges::reverse_view`.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

I don't think I need to see this again after you address my comments, and have green CI. Thanks!



================
Comment at: libcxx/include/__ranges/reverse_view.h:20-21
+
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
----------------
Not needed since we don't use `min`/`max`.


================
Comment at: libcxx/include/__ranges/reverse_view.h:35
+    using _Cache = _If<_UseCache, __non_propagating_cache<reverse_iterator<iterator_t<_View>>>, __empty_cache>;
+    [[no_unique_address]] _Cache __cached_begin_ = _Cache();
+    _View __base_ = _View();
----------------
Per our discussion just now, could you file a bug report against Clang to ask whether this is intended: https://godbolt.org/z/bs15aq466?


================
Comment at: libcxx/include/__ranges/reverse_view.h:36
+    [[no_unique_address]] _Cache __cached_begin_ = _Cache();
+    _View __base_ = _View();
+
----------------
Why not `[[no_unique_address]]`?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.reverse/base.pass.cpp:42
+  }
+  {
+    auto rev = std::ranges::reverse_view(BidirSentRange<MoveOnly>{buffer});
----------------
Please comment `// test with a non-common_range` below, and `// test with a common_range` above. This applies elsewhere too - I'd like to be able to tell exactly what you're testing by just glancing at it - imagine I'm veeeery lazy.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.reverse/begin.pass.cpp:44
+  int buffer[8] = {1, 2, 3, 4, 5, 6, 7, 8};
+
+  {
----------------
Can we add a test with a random access range that is *not* a common range? We won't be caching in that case either.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.reverse/begin.pass.cpp:82
+    // Make sure we cache begin.
+    CountedView view{buffer};
+    std::ranges::reverse_view rev(view);
----------------
As we just discussed, this test doesn't fail properly even if we remove the caching inside `reverse_view`, so that should be fixed.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.reverse/ctor.view.pass.cpp:42
+    assert(rev.base().ptr_ == buffer);
+  }
+
----------------
Can you test the explicit-ness by doing 
```
static_assert( std::is_constructible_v<...>);
static_assert(!std::is_convertible_v<...>);
```


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.reverse/end.pass.cpp:57
+  {
+    // TODO: sfinae BidirSentRange<Copyable>{buffer}
+  }
----------------
This looks like a copy-paste leftover from the tests for `begin` (quoting yourself). Let's remove.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.reverse/range_concept_conformance.compile.pass.cpp:24-27
+static_assert(std::ranges::bidirectional_range<std::ranges::reverse_view<test_view<bidirectional_iterator>>>);
+static_assert(std::ranges::random_access_range<std::ranges::reverse_view<test_view<random_access_iterator>>>);
+static_assert(std::ranges::random_access_range<std::ranges::reverse_view<test_view<contiguous_iterator>>>);
+static_assert(!std::ranges::contiguous_range<std::ranges::reverse_view<test_view<contiguous_iterator>>>);
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107096